This is an archive of the discontinued LLVM Phabricator instance.

[darwin][asan] add test for application specific information in crash logs
ClosedPublic

Authored by aralisza on Feb 15 2021, 4:48 PM.

Details

Summary

Added a lit test that finds its corresponding crash log and checks to make sure it has asn output under Application Specific Information.

This required adding two python commands:

  • get_pid_from_output: takes the output from the asan instrumentation and parses out the process ID
  • print_crashreport_for_pid: takes in the pid of the process and the file name of the binary that was run and prints the contents of the corresponding crash log.

This test was added in preparation for changing the integration with crash reporter from the old api to the new api, which is implemented in a subsequent commit.

rdar://69767688

Diff Detail

Event Timeline

aralisza requested review of this revision.Feb 15 2021, 4:48 PM
aralisza created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2021, 4:48 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
delcypher added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
785 ↗(On Diff #323843)

We should move this check and #include to near the top of the file. The problem with doing the #include here is that we're currently in the __sanitizer namespace. Many header files don't expect to be included inside a namespace and may misbehave. It's probably okay here because we're only using macros but to avoid potential problems in the future let's move the include to where all the others are near the top of the file outside of the __sanitizer namespace.

788 ↗(On Diff #323843)

Both the #if ... and #else sides contain

extern "C" {

it's probably cleaner to hoist this out to make simpler code, i..e.:

extern "C" {
#if defined(HAVE_CRASHREPORTERCLIENT_H)
CRASH_REPORTER_CLIENT_HIDDEN
struct crashreporter_annotations_t gCRAnnotations
    __attribute__((section("__DATA," CRASHREPORTER_ANNOTATIONS_SECTION))) = {
        CRASHREPORTER_ANNOTATIONS_VERSION,
        0,
        0,
        0,
        0,
        0,
        0,
#if CRASHREPORTER_ANNOTATIONS_VERSION > 4
        0,
#endif
};
#else
// fall back to old crashreporter api
static const char *__crashreporter_info__ __attribute__((__used__)) =
    &__crashreporter_info_buff__[0];
asm(".desc ___crashreporter_info__, 0x10");

#endif
}
806 ↗(On Diff #323843)

Have you tested that this old path compiles and works on macOS?

815 ↗(On Diff #323843)

We should probably move the mutex declaration to be global and not a static variable.

  1. In newer versions of C++/C where this is thread safe it apparently compiles down to some libc++ internals. I'd rather not pull in the dependency. See https://iamroman.org/blog/2017/04/cpp11-static-init/
  1. (Minor reason as we con't compile with older C++ versions anymore). In older versions of the C++ language initializing static variables was racey (undefined behaviour). This is a little silly given that the goal of the mutex is to prevent races.
817 ↗(On Diff #323843)

This "call" here is technically a race because it's not guarded by the mutex. It probably happens to work because all threads would write the same value if there were a race.

We should call CRSetCrashLogMessage after the mutex is held.

compiler-rt/lib/sanitizer_common/sanitizer_mac.h
68 ↗(On Diff #323843)

I don't think we need the inline keyword here. I don't think it really adds that much because:

  • Outside of sanitizer_mac.cpp we won't be able to inline unless we build with link-time-optimization (LTO) (I don't think we do that)
  • Inside sanitizer_mac.cpp the benefits of inlining calls to CRAppendCrashLogMessage are a bit unknown at this point because I don't we've measure the perf impact, have we?

I suggest we drop the inline keyword and add it in a separate patch if we discover there's a perf problem.

compiler-rt/test/asan/TestCases/Darwin/asan_log_to_crashreporter.cpp
2

Is this a copy-paste error? This comment doesn't make sense to me because we are modifying ASAN_OPTIONS by the using the %env_asan_opts substitution.

2

We should mark this test with.

UNSUPPORTED: ios

until we've made it possible to run on iOS.

2

I think we should introduce the test as a separate patch first. Then as a second patch do the api change.

9

Could we do something like?

RUN: TEST_PID="$(%ios_command get_pid_from_output.py <%t.process_output.txt)" ; \
RUN: %ios_command print_crashreport_for_pid.py --filename=%basename_t.tmp --pid=$(TEST_PID) | FileCheck %s

This makes it much more obvious how the PID is getting passed?

18

Side note. If the crash reporter prefs are set to "Server", do the crash logs still show up?

compiler-rt/test/lit.common.cfg.py
599

As we discussed I'm not a huge fan of this.

  1. %ios_command is a bit confusing because we aren't using this command on iOS (yet).
  2. The current precedent it to use separate substitutions for each command introduced..
  3. We shouldn't hardcode python here because the CMake build might have been configured to use a different python install. You can probably use config.python_executable here (see lit.common.configured.in). Take a look at how %asan_symbolize is set.
compiler-rt/test/sanitizer_common/ios_commands/get_pid_from_output.py
2

Maybe add a doc string explaining what this script does?

compiler-rt/test/sanitizer_common/ios_commands/print_crashreport_for_pid.py
2

Maybe add a doc string explaining what this script does?

7

Filename is a little misleading here. It's actually the filename prefix rather than the whole filename. Rather than exposing this implementation detail maybe this option should be called --binary-filename ?

15

I'd suggest removing this part. Parsing the pid on stdin is not very intuitive. maybe we should make passing the pid a mandatory argument?

20

The retry_count should probably be settable from the command line so we can conveniently change it from a test.

27

This variable back-off strategy probably deserves a comment.

aralisza updated this revision to Diff 324161.Feb 16 2021, 6:53 PM
aralisza marked 17 inline comments as done.

most of the code review comments

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
806 ↗(On Diff #323843)

Yes, the test I wrote passes on this path on my machine.

compiler-rt/test/asan/TestCases/Darwin/asan_log_to_crashreporter.cpp
18

Yup!

From reading the description of that setting, it should only change whether the popup dialog window shows up or not and does not affect whether the crashlog itself is created.

aralisza updated this revision to Diff 324162.Feb 16 2021, 6:55 PM

actual change

aralisza updated this revision to Diff 324170.Feb 16 2021, 7:19 PM

remove implementation changes

aralisza retitled this revision from [draft] currently working to [darwin][asan] add test for application specific information in crash logs.Feb 16 2021, 7:26 PM
aralisza edited the summary of this revision. (Show Details)
aralisza edited the summary of this revision. (Show Details)

Moved implementation changes to a stacked diff

aralisza marked an inline comment as done.Feb 16 2021, 7:29 PM
aralisza added reviewers: delcypher, yln, kubamracek.
delcypher added inline comments.Feb 16 2021, 8:01 PM
compiler-rt/test/lit.common.cfg.py
599

This will likely fail if any the paths have spaces in them. You probably want to wrap config.python_executable and get_ios_commands_dir() in a call to sh_quote() (defined in the asan lit config file right now).

compiler-rt/test/sanitizer_common/ios_commands/print_crashreport_for_pid.py
15

Is pid actually mandatory? I don't see anything in the script that enforces that. It just looks like we won't find any crash logs after searching through all the files. We should probably just error out earlier if the pid isn't provided.

32

I wonder if this approach will be reliable enough when the machine is under heavy load.

tries = 10
t = 2.0
acc = 0
for x in range(1, tries+1):
  acc += (t / x)
print("total: {}".format(acc))
...
total: 5.8579365079365076

So the maximum time we'll wait here is just under 6 seconds. I wonder if that's long enough. Of course scanning the disk and opening the log files might end up actually dominating execution time if there are a lot of crash logs on the machine.

Harbormaster completed remote builds in B89466: Diff 324162.
aralisza added inline comments.Feb 17 2021, 11:52 AM
compiler-rt/test/sanitizer_common/ios_commands/print_crashreport_for_pid.py
32

I'll increase t=5 so the total time will be ~15 seconds. Hopefully that'll be enough

aralisza updated this revision to Diff 324378.Feb 17 2021, 11:53 AM

make pid and binary filename required and increase time to wait between retries

This revision is now accepted and ready to land.Feb 17 2021, 1:10 PM
aralisza edited the summary of this revision. (Show Details)Feb 17 2021, 1:39 PM
aralisza updated this revision to Diff 325122.Feb 19 2021, 5:33 PM

disable lit test and add radar number

aralisza updated this revision to Diff 325575.Feb 22 2021, 2:40 PM

line length fix