This is an archive of the discontinued LLVM Phabricator instance.

[asan] Add flag (-external_symbolizer_path_from_binary) to find llvm-symbolizer relative to the binary's directory.
ClosedPublic

Authored by akhuang on Jan 12 2021, 4:20 PM.

Details

Summary

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

Diff Detail

Event Timeline

akhuang requested review of this revision.Jan 12 2021, 4:20 PM
akhuang created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2021, 4:20 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
rnk added a comment.Jan 13 2021, 11:55 AM

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

akhuang updated this revision to Diff 316765.Jan 14 2021, 1:21 PM

-Add lit test, although it uses a hard-coded path
-Make -external_symbolizer_path take precedence

akhuang marked 2 inline comments as done.Jan 14 2021, 2:57 PM
rnk added a comment.Jan 19 2021, 2:23 PM

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_symbolizer_posix_libcdep.cpp
404

+1 for just !path

compiler-rt/test/asan/TestCases/external-symbolizer-path.cpp
5 ↗(On Diff #316765)

consider changing directory before running to make sure that this path is not a valid relative path from $PWD

17 ↗(On Diff #316765)

external-symbolizer-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.
Maybe something like internal var: external_symbolizer_path=@BINARY_DIR/my/relative/path ?

akhuang marked 4 inline comments as done.Jan 20 2021, 9:21 AM
akhuang added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_flags.inc
27

That seems cleaner I guess, something like replacing "@BINARY_DIR" with path to binary?

vitalybuka added inline comments.Jan 20 2021, 11:05 AM
compiler-rt/lib/sanitizer_common/sanitizer_flags.inc
27

yes
@eugenis @rnk WDYT?

rnk added inline comments.Jan 20 2021, 11:23 AM
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.

akhuang added inline comments.Jan 20 2021, 11:38 AM
compiler-rt/lib/sanitizer_common/sanitizer_flags.inc
27

Yeah, I was thinking just as a prefix.

akhuang updated this revision to Diff 317960.Jan 20 2021, 11:49 AM

-switch to using @BINARY_DIR
-update test case (add directory change, fix name)

vitalybuka added inline comments.Jan 20 2021, 12:48 PM
compiler-rt/lib/sanitizer_common/sanitizer_flags.inc
27

I have no opinion on prefix vs any position.

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp
403

static const char kPrefix[] = "@BINARY_DIR";
f (internal_strncmp(path, kPrefix, ARRAY_SIZE(kPrefix) - 1) == 0) {

vitalybuka added inline comments.Jan 20 2021, 12:53 PM
compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp
403

Also I don't know if @ it the best char.
we definitely should not use $ or % to avoid complex escaping in shell scripts.
If you know LLVM uses similar placeholders maybe we should consider similar style.

akhuang updated this revision to Diff 317989.Jan 20 2021, 1:15 PM

clean up string code

vitalybuka added inline comments.Jan 21 2021, 9:02 PM
compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp
405

internal_strlen needs to scan string
ARRAY_SIZE() - 1 is constant.

compiler-rt/test/asan/TestCases/external-symbolizer-path.cpp
1 ↗(On Diff #317989)

// REQUIRES: shell ?

11 ↗(On Diff #317989)

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.

akhuang updated this revision to Diff 318592.Jan 22 2021, 11:30 AM

-change string len to use ARRAY_SIZE
-move test to sanitizer_common

akhuang marked 2 inline comments as done.Jan 22 2021, 11:36 AM
akhuang added inline comments.
compiler-rt/test/asan/TestCases/external-symbolizer-path.cpp
11 ↗(On Diff #317989)

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
2

"REQUIRES: linux" should not be needed with "// REQUIRES: shell"

20

we need msan_default_options, ubsan_default_options etc.
sanitizer_common tests triggered by run "ninja check-sanitizer", I guess everything but asan fail this way.

28

this may not trigge any error with other sanitizers
just add #include <sanitizer/common_interface_defs.h> and call __sanitizer_symbolize_pc, you can find examples in test/sanitizer_common/TestCases/symbolize_pc.cpp

akhuang updated this revision to Diff 319172.Jan 25 2021, 5:05 PM
akhuang marked an inline comment as done.

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
28

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?

vitalybuka added inline comments.Jan 26 2021, 9:43 PM
compiler-rt/test/sanitizer_common/TestCases/external_symbolizer_path.cpp
28

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

akhuang updated this revision to Diff 319960.Jan 28 2021, 2:07 PM

Update test

compiler-rt/test/sanitizer_common/TestCases/external_symbolizer_path.cpp
28

oh, sorry, my code was just broken, everything works now.

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.

have you looked at this? Maybe we need tiny fix to make existing stuff.

akhuang marked an inline comment as done.Feb 1 2021, 12:39 PM

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.

have you looked at this? Maybe we need tiny fix to make existing stuff.

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 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.

have you looked at this? Maybe we need tiny fix to make existing stuff.

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?

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
36

you can avoid __*_default_options using %env_tool_opts=external_symbolizer_path..." in RUN: line

akhuang updated this revision to Diff 320954.Feb 2 2021, 4:58 PM
akhuang marked an inline comment as done.

Use %env_tools_opt in test case, change to %d for consistency

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.

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.
If still like to make substitution here please still use SubstituteForFlagValue() and add %d there so we
have single point which responsible for that.

akhuang updated this revision to Diff 321111.Feb 3 2021, 8:59 AM

Move substitution into SubstituteForFlagValue.

Sounds good, I added the substitution to SubstituteForFlagValue, and am still calling that from the symbolizer code.

vitalybuka added inline comments.Feb 4 2021, 1:07 AM
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 updated this revision to Diff 321507.Feb 4 2021, 10:11 AM
akhuang marked 2 inline comments as done.

address comments

akhuang updated this revision to Diff 321512.Feb 4 2021, 10:45 AM

fix some typos

vitalybuka accepted this revision.Feb 4 2021, 12:37 PM

LGTM we few comments

compiler-rt/lib/sanitizer_common/sanitizer_flags.cpp
68–78 ↗(On Diff #321111)

not done?

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_win.cpp
294

user_path?

This revision is now accepted and ready to land.Feb 4 2021, 12:37 PM
akhuang updated this revision to Diff 321550.Feb 4 2021, 12:56 PM
akhuang marked an inline comment as done.

address comments

vitalybuka accepted this revision.Feb 4 2021, 2:40 PM

thanks for helping / reviewing!

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

Thanks - the bot is green again.