Skip to content

[lldb] Fix incorrect logical operator in 'if' condition check (NFC) #94779

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 25, 2024

Conversation

xgupta
Copy link
Contributor

@xgupta xgupta commented Jun 7, 2024

The condition checking for missing class name, interpreter dictionary, and script object incorrectly used logical AND (&&), which could never be true to enter the 'if' block.

This commit uses separate if conditions for each class name, interpreter dictionary, and script object.

Cought by cppcheck -
lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h:89:11: warning: Identical inner 'if' condition is always true. [identicalInnerCondition]
lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h:91:16: warning: Identical inner 'if' condition is always true. [identicalInnerCondition]

Fix #89195

@xgupta xgupta requested a review from medismailben June 7, 2024 17:55
@xgupta xgupta requested a review from JDevlieghere as a code owner June 7, 2024 17:55
@llvmbot llvmbot added the lldb label Jun 7, 2024
@llvmbot
Copy link
Member

llvmbot commented Jun 7, 2024

@llvm/pr-subscribers-lldb

Author: Shivam Gupta (xgupta)

Changes

Source code analyser cppcheck says:
lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h:89:11: warning: Identical inner 'if' condition is always true. [identicalInnerCondition]
lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h:91:16: warning: Identical inner 'if' condition is always true. [identicalInnerCondition]

Fix #89195


Full diff: https://github.com/llvm/llvm-project/pull/94779.diff

1 Files Affected:

  • (modified) lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h (+1-1)
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h
index 163659234466d..30811639c9a95 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h
@@ -85,7 +85,7 @@ class ScriptedPythonInterface : virtual public ScriptedInterface {
     bool has_class_name = !class_name.empty();
     bool has_interpreter_dict =
         !(llvm::StringRef(m_interpreter.GetDictionaryName()).empty());
-    if (!has_class_name && !has_interpreter_dict && !script_obj) {
+    if (!has_class_name || !has_interpreter_dict || !script_obj) {
       if (!has_class_name)
         return create_error("Missing script class name.");
       else if (!has_interpreter_dict)

Copy link
Member

@JDevlieghere JDevlieghere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comment as #94775 (comment)

@xgupta xgupta changed the title [LLDB][NFC] Fix a cppcheck warning in Python/Interfaces/ScriptedPythonInterface.h [lldb] Fix incorrect logical operator in 'if' condition check (NFC) Jun 8, 2024
@xgupta
Copy link
Contributor Author

xgupta commented Jun 15, 2024

gentle ping!

@xgupta
Copy link
Contributor Author

xgupta commented Jul 24, 2024

ping for review comment.

Copy link
Member

@bulbazord bulbazord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good to me. Please make sure you update the commit message so it accurately reflects the change since there were a few revisions. :)

@xgupta xgupta merged commit 2ba3fe7 into llvm:main Jul 25, 2024
5 checks passed
@llvm-ci
Copy link
Collaborator

llvm-ci commented Jul 25, 2024

LLVM Buildbot has detected a new failure on builder lldb-x86_64-debian running on lldb-x86_64-debian while building lldb at step 6 "test".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/2857

Here is the relevant piece of the build log for the reference:

Step 6 (test) failure: build (failure)
...
PASS: lldb-api :: python_api/module_section/TestModuleAndSection.py (88 of 2633)
PASS: lldb-api :: commands/expression/timeout/TestCallWithTimeout.py (89 of 2633)
PASS: lldb-api :: commands/expression/persistent_result/TestPersistentResult.py (90 of 2633)
PASS: lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx-simulators/string/TestDataFormatterLibcxxStringSimulator.py (91 of 2633)
PASS: lldb-api :: commands/gui/breakpoints/TestGuiBreakpoints.py (92 of 2633)
PASS: lldb-api :: python_api/watchpoint/watchlocation/TestTargetWatchAddress.py (93 of 2633)
PASS: lldb-api :: tools/lldb-dap/breakpoint/TestDAP_setBreakpoints.py (94 of 2633)
PASS: lldb-api :: api/listeners/TestListener.py (95 of 2633)
PASS: lldb-api :: functionalities/step-vrs-interrupt/TestStepVrsInterruptTimeout.py (96 of 2633)
PASS: lldb-api :: tools/lldb-server/TestGdbRemote_vCont.py (97 of 2633)
FAIL: lldb-api :: functionalities/postmortem/mach-core/TestMachCore.py (98 of 2633)
******************** TEST 'lldb-api :: functionalities/postmortem/mach-core/TestMachCore.py' FAILED ********************
Script:
--
/usr/bin/python3 /home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/test/API/dotest.py -u CXXFLAGS -u CFLAGS --env ARCHIVER=/usr/bin/ar --env OBJCOPY=/usr/bin/objcopy --env LLVM_LIBS_DIR=/home/worker/2.0.1/lldb-x86_64-debian/build/./lib --env LLVM_INCLUDE_DIR=/home/worker/2.0.1/lldb-x86_64-debian/build/include --env LLVM_TOOLS_DIR=/home/worker/2.0.1/lldb-x86_64-debian/build/./bin --arch x86_64 --build-dir /home/worker/2.0.1/lldb-x86_64-debian/build/lldb-test-build.noindex --lldb-module-cache-dir /home/worker/2.0.1/lldb-x86_64-debian/build/lldb-test-build.noindex/module-cache-lldb/lldb-api --clang-module-cache-dir /home/worker/2.0.1/lldb-x86_64-debian/build/lldb-test-build.noindex/module-cache-clang/lldb-api --executable /home/worker/2.0.1/lldb-x86_64-debian/build/./bin/lldb --compiler /home/worker/2.0.1/lldb-x86_64-debian/build/./bin/clang --dsymutil /home/worker/2.0.1/lldb-x86_64-debian/build/./bin/dsymutil --llvm-tools-dir /home/worker/2.0.1/lldb-x86_64-debian/build/./bin --lldb-obj-root /home/worker/2.0.1/lldb-x86_64-debian/build/tools/lldb --lldb-libs-dir /home/worker/2.0.1/lldb-x86_64-debian/build/./lib -t /home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/test/API/functionalities/postmortem/mach-core -p TestMachCore.py
--
Exit Code: 1

Command Output (stdout):
--
lldb version 20.0.0git (https://github.com/llvm/llvm-project.git revision 2ba3fe7356f065757a2279f65e4ef5c8f1476293)
  clang revision 2ba3fe7356f065757a2279f65e4ef5c8f1476293
  llvm revision 2ba3fe7356f065757a2279f65e4ef5c8f1476293
Skipping the following test categories: ['libc++', 'dsym', 'gmodules', 'debugserver', 'objc']

--
Command Output (stderr):
--
Change dir to: /home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/test/API/functionalities/postmortem/mach-core
runCmd: settings clear -all

output: 

runCmd: settings set symbols.enable-external-lookup false

output: 

runCmd: settings set target.inherit-tcc true

output: 

runCmd: settings set target.disable-aslr false

output: 

runCmd: settings set target.detach-on-error false

output: 


@DavidSpickett
Copy link
Collaborator

The code looks ok but as landed it turned A && B && C into A || B || C, despite that looking logical.

@medismailben can you figure out the intent here, I'm assuming this was your code originally.

yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
… (NFC) (#94779)

Summary:
The condition checking for missing class name, interpreter dictionary,
and script object incorrectly used logical AND (&&), which could never
be true to enter the 'if' block.

This commit uses separate if conditions for each class name, interpreter
dictionary, and script object.

Cought by cppcheck -

lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h:89:11:
warning: Identical inner 'if' condition is always true.
[identicalInnerCondition]

lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h:91:16:
warning: Identical inner 'if' condition is always true.
[identicalInnerCondition]

Fix #89195

---------

Co-authored-by: Shivam Gupta <[email protected]>

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60250553
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
@xgupta xgupta deleted the fix89195 branch February 10, 2025 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h:89: possible && and || mixup ?
7 participants