Page MenuHomePhabricator

[sanitizers] Force pickup of new symbolizer (round #2).
AbandonedPublic

Authored by hctim on Dec 17 2020, 4:24 PM.

Details

Summary

It's possible currently that the sanitizer runtimes when testing grab
the path to the symbolizer through *SAN_SYMBOLIZER_PATH=...

This can be polluted by things like Android's setup script. This patch
forces external_symbolizer_path=$new_build_out_dir/llvm-symbolizer when
%env_tool_options is used.

Same principle with Android, except that we need to replace the path to
the new symbolizer with the path to the new symbolizer on the device.

Previously committed, fixed-forward, then reverted to bring us back to
stable before the weekend.

Diff Detail

Event Timeline

hctim requested review of this revision.Dec 17 2020, 4:24 PM
hctim created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 17 2020, 4:24 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
oontvoo added inline comments.
compiler-rt/test/sanitizer_common/android_commands/android_common.py
41

Maybe test -e ?

( or -r since "exists" isn't the same as readable? or executable?)

vitalybuka added inline comments.Dec 22 2020, 8:43 PM
compiler-rt/test/sanitizer_common/android_commands/android_common.py
37

do not add unneeded args

40

why not just call adb(['shell', 'ls', ....]);

compiler-rt/test/sanitizer_common/android_commands/android_run.py
12

why do we need device_file_exists at all?
can we just make sure that it's always there?

hctim marked 4 inline comments as done.Jan 5 2021, 4:47 PM
hctim added inline comments.
compiler-rt/test/sanitizer_common/android_commands/android_common.py
41

good idea, test -x done.

compiler-rt/test/sanitizer_common/android_commands/android_run.py
12

The symbolizer needs to be cross-compiled, which isn't easy (possible?) to accomplish with a standalone CRT build. The buildbot script always pushes it, but I'd rather not force people to use the buildbot script.

hctim updated this revision to Diff 314754.Jan 5 2021, 4:47 PM
hctim marked 2 inline comments as done.
  • Changed to 'test -x' instead of 'ls' in test, and removed timeout blob.
vitalybuka added inline comments.Jan 6 2021, 3:03 PM
compiler-rt/test/sanitizer_common/android_commands/android_common.py
40

you probably marked this one as done by mistake?

compiler-rt/test/sanitizer_common/android_commands/android_run.py
12

So what is going to happen if has_symbolizer is false? Most tests will fail?

hctim marked 2 inline comments as done.Jan 6 2021, 3:14 PM
hctim added inline comments.
compiler-rt/test/sanitizer_common/android_commands/android_common.py
40

thanks for the catch

compiler-rt/test/sanitizer_common/android_commands/android_run.py
12

We remove any metion of external_symbolizer_path=... from *SAN_OPTIONS. So sanitizer-common will do the normal thing of looking in the CWD and PATH and will likely fall back to addr2line/no symbolizer as normal. Tests should continue as usual (just they don't have the symbolizer). I figured this was a better option than forcing that tests can only be run on devices that have the symbolizer.

hctim updated this revision to Diff 315006.Jan 6 2021, 3:14 PM
hctim marked 2 inline comments as done.
  • Remove timeout command from 'test'.
oontvoo added inline comments.Jan 6 2021, 3:36 PM
compiler-rt/test/sanitizer_common/android_commands/android_common.py
40

you probably marked this one as done by mistake?

I think vitalybuka@ was asking why you needed to do a manual subprocess.call(), as opposed to just using the the adb() function already defined (line 16)

something like adb(['shell', 'test', '-x', path]) ?

hctim marked an inline comment as done.Jan 6 2021, 4:04 PM
hctim added inline comments.
compiler-rt/test/sanitizer_common/android_commands/android_common.py
40

ah, because adb() "fails" if the retcode is non-zero, and we want to observe the value of the retcode.

vitalybuka added inline comments.Jan 6 2021, 5:36 PM
compiler-rt/test/sanitizer_common/android_commands/android_common.py
41

I guess the following should be a part of adb() then
it returns exit code so caller should do them
maybe extract some adb_internal() and use it in adb and in device_file...?

if ret != 0:
  print "adb command failed", args
  print tmpname
  out.close()
  out = open(tmpname, 'r')
  print out.read()
compiler-rt/test/sanitizer_common/android_commands/android_run.py
12

do they actually work? I suspect this case is not supported at all and you unnecessary complicate the code.
if so you can remove device_file_exists from this patch.

Another concern is testing time. How longer to it takes to execute all tests with device_file_exists and without?

hctim abandoned this revision.Jan 7 2021, 8:59 AM

Scratch this patch - polluted environment probably doesn't necessitate a workaround that increases the complexity of the runner.