We want way to set a path to llvm-symbolizer that isn't relative
to the current working directory; this change adds a flag that takes a path
relative to the current binary.
This approach came from comments in https://reviews.llvm.org/D93070
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Please add a lit test for this. The test could actually copy llvm-symbolizer into a temporary directory, and you could embed the option in __asan_default_options similar to what we want to do in Chromium. I think it's also reasonable to test that if llvm-symbolizer doesn't exist, we get a warning, not an error.
compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp | ||
---|---|---|
404 | If both options are set, we should prefer the external_symbolizer_path. ClusterFuzz for example sets it explicitly. | |
compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_win.cpp | ||
294 | ditto |
-Add lit test, although it uses a hard-coded path
-Make -external_symbolizer_path take precedence
Super nits
@eugenis @vitalybuka, are you OK with this option?
compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp | ||
---|---|---|
404 | nit, maybe just !path | |
compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_win.cpp | ||
294 | ditto, !user_path |
LGTM
compiler-rt/lib/sanitizer_common/sanitizer_flags.inc | ||
---|---|---|
27 | as-is priority of these flags is not clear. Maybe add check that only one can be !null. I guess this one can be relative to the current dir. |
compiler-rt/lib/sanitizer_common/sanitizer_flags.inc | ||
---|---|---|
27 | That seems cleaner I guess, something like replacing "@BINARY_DIR" with path to binary? |
compiler-rt/lib/sanitizer_common/sanitizer_flags.inc | ||
---|---|---|
27 | Works for me. Should it be supported anywhere in the string, or only as a prefix? I lean towards only supporting it as a prefix. |
compiler-rt/lib/sanitizer_common/sanitizer_flags.inc | ||
---|---|---|
27 | Yeah, I was thinking just as a prefix. |
compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp | ||
---|---|---|
403 | Also I don't know if @ it the best char. |
compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp | ||
---|---|---|
405 | internal_strlen needs to scan string | |
compiler-rt/test/asan/TestCases/external-symbolizer-path.cpp | ||
1 | // REQUIRES: shell ? | |
11 | we are changing sanitizer_common so the test probably should be there There __sanitizer_symbolize_pc which can be used to trigger symbolizer from any sanitizer. |
compiler-rt/test/asan/TestCases/external-symbolizer-path.cpp | ||
---|---|---|
11 | I copied the test to sanitizer_common and it seems to be working as is - is __sanitizer_symbolize_pc needed? |
I was going to advise to move substitution into flags.cpp and spotted SubstituteForFlagValue
But it does not work as-is, I didn't yet checked why.
If so, it would be still useful to fix it and finish the test.
compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp | ||
---|---|---|
411 | we leak this internal_strdup? | |
compiler-rt/test/sanitizer_common/TestCases/external_symbolizer_path.cpp | ||
1 ↗ | (On Diff #318592) | "REQUIRES: linux" should not be needed with "// REQUIRES: shell" |
19 ↗ | (On Diff #318592) | we need msan_default_options, ubsan_default_options etc. |
27 ↗ | (On Diff #318592) | this may not trigge any error with other sanitizers |
address comments
compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp | ||
---|---|---|
411 | oh, right, guess we don't need that | |
compiler-rt/test/sanitizer_common/TestCases/external_symbolizer_path.cpp | ||
27 ↗ | (On Diff #318592) | for some reason the tests with __sanitizer_symbolize_pc are hanging when I try to run them - do you know why that might be happening? |
compiler-rt/test/sanitizer_common/TestCases/external_symbolizer_path.cpp | ||
---|---|---|
27 ↗ | (On Diff #318592) | No. Which sanitizer? It should be possible to extract the command line from test and run under gdb. alternative is to raise(SIGSEGV); it should trigger stack trace with all sanitizers |
Update test
compiler-rt/test/sanitizer_common/TestCases/external_symbolizer_path.cpp | ||
---|---|---|
27 ↗ | (On Diff #318592) | oh, sorry, my code was just broken, everything works now. |
Just looked and it seems like SubstituteForFlagValue only runs for include/include_if_exists flags? I could do a similar thing for ParseString, don't know if we want to do that? Then I guess the @BINARY_DIR substitution would work for all the flags.
I think we should handle symbolizer path next to RegisterIncludeFlags in a similar way.
Having that we have %b we should not add @BINARY_DIR with a same meaning.
I think we should handle symbolizer path next to RegisterIncludeFlags in a similar way.
Having that we have %b we should not add @BINARY_DIR with a same meaning.
%b is not quite the same, though, it's just the name of the binary without the path. And looks like the include flags are handled differently from things like the symbolizer_path flag. We could use a similar naming scheme, like %d or something?
%d lgtm
compiler-rt/test/sanitizer_common/TestCases/external_symbolizer_path.cpp | ||
---|---|---|
35 ↗ | (On Diff #319960) | you can avoid __*_default_options using %env_tool_opts=external_symbolizer_path..." in RUN: line |
I ended up not moving things into SubstituteForFlagValue because there are apparently other flags that can contain things like '%p' that shouldn't be substituted.
I changed it from @BINARY_DIR to %d. Thinking about it more, maybe @BINARY_DIR is clearer? And for now it's only being used for external_symbolizer_path. But both seem fine to me.
SubstituteForFlagValue is not yet applied to all flags, just only to two "include" flags. I assumed you are going to add new RegisterHandler, but then you need to split flag declaration in class and actual handler registration and also there are direct assignment like "cf.external_symbolizer_path = GetEnv("HWASAN_SYMBOLIZER_PATH");" which miss the parser.
So I am fine with substitution in ChooseSymbolizerTools but please still use SubstituteForFlagValue() and add %d there so we have a single point which responsible for that.
And I still prefer a little bit "%d" over "@BIN_DIR" just for consistency.
compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_win.cpp | ||
---|---|---|
296 | I would prefer you still use RegisterHandler, but then you need to split flag declaration in class and actual handler registration. |
Sounds good, I added the substitution to SubstituteForFlagValue, and am still calling that from the symbolizer code.
compiler-rt/lib/sanitizer_common/sanitizer_flags.cpp | ||
---|---|---|
68–78 ↗ | (On Diff #321111) | |
compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp | ||
404–408 | Substituting only if the first is % looks inconsistent, please do it always. | |
compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_win.cpp | ||
294 | same | |
compiler-rt/lib/sanitizer_common/sanitizer_win.cpp | ||
1052–1062 |
@akhuang This buildbot is still red after these changes - please can you take a look?
http://lab.llvm.org:8011/#/builders/112/builds/3626
...
http://lab.llvm.org:8011/#/builders/112/builds/3809
I'm not entirely sure why the buildbots are failing - I think I'll just disable the test everywhere for now.
This bot uses -DBUILD_SHARED_LIBS=ON So it fails to load libLLVMOption.so which was not copied with llvm-symbolizer. I guess we don't want to copy so files.
Should be fixed with
REQUIRES: static-libs
as-is priority of these flags is not clear. Maybe add check that only one can be !null.
I guess this one can be relative to the current dir.
Maybe something like internal var: external_symbolizer_path=@BINARY_DIR/my/relative/path ?