Page MenuHomePhabricator

[ubsan] support print_module_map flag in standalone mode
ClosedPublic

Authored by aralisza on Mar 1 2021, 4:33 PM.

Details

Summary

Currently, print_module_map is only respected for ubsan if it is ran in tandem with asan. This patch adds support for this flag in standalone mode. I copied the pattern used to implement this for asan.

Also added a common print_module_map lit test for Darwin only. Since the print messages are different per platform, we need to write a regex test to cover them. This test is coming in a separate patch

rdar://56135732

Diff Detail

Event Timeline

aralisza requested review of this revision.Mar 1 2021, 4:33 PM
aralisza created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2021, 4:33 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
aralisza edited the summary of this revision. (Show Details)Mar 1 2021, 4:35 PM
aralisza edited the summary of this revision. (Show Details)
aralisza edited the summary of this revision. (Show Details)Mar 1 2021, 4:49 PM
vsk accepted this revision.Mar 1 2021, 5:51 PM

Thanks, this looks good to me with a nit. I haven't worked on sanitizer_common very much, so please wait for another +1.

compiler-rt/test/sanitizer_common/TestCases/Darwin/print-module-map.cpp
29

The '{{ . }}' syntax should only be necessary when matching a regex. You should be able to simply write:

// CHECK: SUMMARY:

here and below. The check on line 32 for {{.*ABORTING}} can also just be ABORTING.
(FileCheck does have a mode that's strict about matching full lines, and another separate mode for matching whitespace exactly, but unless something has changed recently those are not on by default.)

This revision is now accepted and ready to land.Mar 1 2021, 5:51 PM
aralisza added 2 blocking reviewer(s): delcypher, vitalybuka.Mar 2 2021, 11:17 AM
This revision now requires review to proceed.Mar 2 2021, 11:17 AM
aralisza updated this revision to Diff 327561.Mar 2 2021, 1:21 PM

remove unecessary braces in lit test

aralisza marked an inline comment as done.Mar 2 2021, 1:21 PM
aralisza edited the summary of this revision. (Show Details)

Since the print messages are different per platform, it would be difficult to write a platform-agnostic test.

So the mac does

Printf("Process module map:\n");

Other POSIX platforms do

Report("Process memory map follows:\n");

and the Windows implementation does

Report("Dumping process modules:\n");

It's unfortunate that these outputs are not consistent. We could write a regex here to try to cover all 3 which means we could re-use this test elsewhere. However, that seems like a pain. Instead I think we should land this patch as it is and then as a separate patch unify what we print when we print the module map and then make this test non Darwin specific.

delcypher accepted this revision.Mar 2 2021, 7:15 PM

LGTM but let's give @vitalybuka time to take a look

vitalybuka added inline comments.Mar 4 2021, 1:30 PM
compiler-rt/test/sanitizer_common/TestCases/Darwin/print-module-map.cpp
11–13

Lets make it xfail, so if it's somehow fixed it, they don't forget to update the test.

17–20

my clang reports warning here
let's make something less obvious

29–36

With pattern like this we can move it into Posix/

vsk added inline comments.Mar 4 2021, 1:36 PM
compiler-rt/test/sanitizer_common/TestCases/Darwin/print-module-map.cpp
17–20

Just curious - what motivates this? Clang is required to emit the ubsan diagnostic even if the UB is detectable statically. And passing -w would hide the warning, if that's important.

vitalybuka added inline comments.Mar 4 2021, 1:39 PM
compiler-rt/test/sanitizer_common/TestCases/Darwin/print-module-map.cpp
17–20

unnecessary noise when looking at text failure.
"-w" is fine as well

aralisza updated this revision to Diff 328596.Mar 5 2021, 11:17 AM

change unsupported platforms to fail the test and pass -w to suppress compiler warning noise

vitalybuka accepted this revision.Mar 5 2021, 12:07 PM

change unsupported platforms to fail the test and pass -w to suppress compiler warning noise

So what's about the rest?
The patch is already LGTM, but it would be nice to have the test coverage for more platforms. Doing that in followup patch is fine as well.

This revision is now accepted and ready to land.Mar 5 2021, 12:07 PM

It's slightly complicated to write a platform-agnostic test because the strings printed are different for each platform. Maybe the right course of action is to make at least the heading of the module map section be uniform? What do you think?

It's slightly complicated to write a platform-agnostic test because the strings printed are different for each platform. Maybe the right course of action is to make at least the heading of the module map section be uniform? What do you think?

There is proposed regexp version which works for linux and should work for OSX. I don't find it complicated at all.

I don't mind to unify, in a separate patch. However "Process module map" does not match Linux output. I didn't check content of darwin output if it's reasonable to rename it into "memory map".

aralisza added a comment.EditedMar 5 2021, 1:47 PM

ah sorry I missed that comment. I'll go ahead and move the test in a separate patch

aralisza edited the summary of this revision. (Show Details)Mar 5 2021, 1:54 PM
This revision was landed with ongoing or failed builds.Mar 5 2021, 2:00 PM
This revision was automatically updated to reflect the committed changes.

It's slightly complicated to write a platform-agnostic test because the strings printed are different for each platform. Maybe the right course of action is to make at least the heading of the module map section be uniform? What do you think?

There is proposed regexp version which works for linux and should work for OSX. I don't find it complicated at all.

I don't mind to unify, in a separate patch. However "Process module map" does not match Linux output. I didn't check content of darwin output if it's reasonable to rename it into "memory map".

@vitalybuka @aralisza I would prefer if we unified the output to be (Process module map) because I have some private code that depends on it but it isn't a dealbreaker for me if we change it. However, if we do change it
we will need to update lib/asan/scripts/asan_symbolize.py because it has code designed to parse the module map as it is currently printed on Darwin.

It's slightly complicated to write a platform-agnostic test because the strings printed are different for each platform. Maybe the right course of action is to make at least the heading of the module map section be uniform? What do you think?

There is proposed regexp version which works for linux and should work for OSX. I don't find it complicated at all.

I don't mind to unify, in a separate patch. However "Process module map" does not match Linux output. I didn't check content of darwin output if it's reasonable to rename it into "memory map".

@vitalybuka @aralisza I would prefer if we unified the output to be (Process module map) because I have some private code that depends on it but it isn't a dealbreaker for me if we change it. However, if we do change it
we will need to update lib/asan/scripts/asan_symbolize.py because it has code designed to parse the module map as it is currently printed on Darwin.

So I don't think this regexp is worth of hassle and I'd keep it as-is.