Skip to content
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

Add lint for nondeterministic pointer comparison #1054

Merged
merged 2 commits into from
Dec 19, 2024

Conversation

nuchi
Copy link
Contributor

@nuchi nuchi commented Oct 2, 2024

@sletz I figured out a CMake integration for the pointer nondeterminism lint I mentioned to you. I did it by wrapping clang-query into a clang plugin, so that a query can be automatically converted into a compiler warning. The plugin is at https://github.com/nuchi/node-matcher-plugin

(Context for others: code generation is nondeterministic because sometimes the compiler will compare pointers in situations where it needs to arbitrarily choose one. But pointers are generated by malloc/new and therefore change from run to run. One way to eliminate this non-determinism is to not compare pointers. But that can be hard to detect. Hence this new lint step.)

Adds a compiler warning for doing a pointer comparison between CTreeBase* or Symbol*, in order to better find where nondeterminism is happening.

It's enabled automatically for make debug, and can be enabled for other targets by doing
make compiler NONDETERMINISM_LINT=yes

Needs llvm-config to be in one's PATH; it'll set the C++ compiler to be the version of clang that's shipped with LLVM. (It builds a clang plugin against the installed LLVM, so it needs to use the same C++ compiler.)

Sample cmake output:

[  1%] Building CXX object CMakeFiles/faust.dir/Users/nuchi/faust/compiler/box_signal_api.cpp.o
In file included from /Users/nuchi/faust/compiler/box_signal_api.cpp:30:
In file included from /Users/nuchi/faust/compiler/global.hh:39:
In file included from /Users/nuchi/faust/compiler/evaluate/loopDetector.hh:33:
In file included from /Users/nuchi/faust/compiler/boxes/boxes.hh:42:
In file included from /Users/nuchi/faust/compiler/tlib/tlib.hh:162:
In file included from /Users/nuchi/faust/compiler/tlib/list.hh:108:
/Users/nuchi/faust/compiler/tlib/tree.hh:181:46: warning: this function call leads to pointer comparison of type CTreeBase *, don't do that
  181 |     void setProperty(Tree key, Tree value) { fProperties[key] = value; }
      |                                              ^~~~~~~~~~~~~~~~
/Users/nuchi/faust/compiler/tlib/tree.hh:182:36: warning: this function call leads to pointer comparison of type CTreeBase *, don't do that
  182 |     void clearProperty(Tree key) { fProperties.erase(key); }
      |                                    ^~~~~~~~~~~~~~~~~~~~~~
/Users/nuchi/faust/compiler/tlib/tree.hh:189:29: warning: this function call leads to pointer comparison of type CTreeBase *, don't do that
  189 |         plist::iterator i = fProperties.find(key);
      |                             ^~~~~~~~~~~~~~~~~~~~~
/Users/nuchi/faust/compiler/tlib/tree.hh:221:22: warning: don't compare pointers of type CTreeBase *
  221 |             } while (new_block < gAllocatedBlock);
      |                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~
4 warnings generated.

Note that it'll be very noisy, because it'll give the same warning for e.g. tree.hh in every translation unit that includes it.

@sletz
Copy link
Member

sletz commented Oct 2, 2024

Thanks, I'll have a look.

@sletz
Copy link
Member

sletz commented Dec 17, 2024

@nuchi I rebased with master-dev and tried to build but could not for now. Could you simply paste somewhere the entire log you get with the lint plugin ? Then I could try to fix the code starting from that.

@nuchi nuchi force-pushed the nondeterministic-pointers branch from 4fd3f8c to 87c523c Compare December 19, 2024 01:22
@nuchi
Copy link
Contributor Author

nuchi commented Dec 19, 2024

Hi @sletz , thanks for letting me know the build failed for you, I rebased on latest and also fixed the issues with git submodule permissions by replacing git@-style submodule with https-style submodule. I also addressed an issue that would have prevented building on linux. If you don't mind would you be willing to try make debug again? I can also take a look at just generating the output for you.

@sletz
Copy link
Member

sletz commented Dec 19, 2024

Thanks. Trying to build on macOS Monterey 12.7.6 with LLCM/Clang 19 installed with macports, I has to do:

export LLVM_DIR=/opt/local/libexec/llvm-19/lib/cmake/llvm
export Clang_DIR=/opt/local/libexec/llvm-19/lib/cmake/clang

but then make debug gives:

/opt/local/bin/cmake --build faustdebug --config Debug 
[  0%] Building CXX object node-matcher-plugin/CMakeFiles/NodeMatcherPlugin.dir/NodeMatcherPlugin.cpp.o
[  0%] Building CXX object node-matcher-plugin/CMakeFiles/NodeMatcherPlugin.dir/QueryParser.cpp.o
/Users/letz/Developpements/faust/node-matcher-plugin/QueryParser.cpp:14:10: fatal error: 'clang/Tooling/NodeIntrospection.h' file not found
   14 | #include "clang/Tooling/NodeIntrospection.h"

Which LLvm/Clang version are you using ?

@sletz
Copy link
Member

sletz commented Dec 19, 2024

OK, compiling with LLVM 18.

@sletz
Copy link
Member

sletz commented Dec 19, 2024

@nuchi I was able to fix all sources of non-determinism, using a somewhat "brute-force" approach on all use of Tree pointer comparisons, either directly, or when used on STL containers. Thanks again for the PR that makes this fix possible !

@nuchi
Copy link
Contributor Author

nuchi commented Dec 19, 2024

great to hear it! I'm happy to leave this branch up to make it easier to periodically rebase and check for accidental relapses if you don't want to merge. sounds like it might introduce maintenance burdens re: updating LLVM.

@sletz sletz merged commit 87c523c into grame-cncm:master-dev Dec 19, 2024
3 checks passed
@sletz
Copy link
Member

sletz commented Dec 19, 2024

Finally merged, version 2.77.1. I guess it will be doable this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants