This is an archive of the discontinued LLVM Phabricator instance.

[asan] Allow re-exec in instrumented unit tests on Darwin (fix unit tests on macOS <=10.10)
ClosedPublic

Authored by kubamracek on Sep 17 2016, 1:59 AM.

Diff Detail

Event Timeline

kubamracek updated this revision to Diff 71728.Sep 17 2016, 1:59 AM
kubamracek retitled this revision from to [asan] Allow re-exec in instrumented unit tests on Darwin (fix unit tests on macOS <=10.10).
kubamracek updated this object.
kubamracek added reviewers: kcc, eugenis, filcab, dvyukov.
kubamracek added a project: Restricted Project.
kubamracek added subscribers: ab, llvm-commits, zaks.anna.
dvyukov added a reviewer: aizatsky.
dvyukov edited edge metadata.

+Alex, +Mike

glider edited edge metadata.Sep 19 2016, 5:20 AM

Sorry, I don't quite understand - do you need to enable re-exec back only for certain OSX versions? If so, shouldn't you check for them?

kubamracek updated this revision to Diff 78064.Nov 15 2016, 1:33 PM
kubamracek edited edge metadata.

Sorry, I don't quite understand - do you need to enable re-exec back only for certain OSX versions? If so, shouldn't you check for them?

That's right. On 10.10 and below, we still need re-exec for unit tests to run correctly. However, it's very hard to do anything in ReexecDisabled(), because it cannot access memory (because it's instrumented and runs too early) and it can't safely call other methods (they might invoke interceptors or instrumented memory accesses). Even if we blacklisted this function, checking the version of OS X is far too complex to be done without function calls. Can we just allow this for all Darwin versions? On 10.11 and newer, re-exec doesn't happen anyway.

Ping. Is this okay to commit?

Alexander is OOO, should return soon.

filcab edited edge metadata.Nov 29 2016, 9:40 AM

IIUC, ReexecDisabled being false on newer systems will just make us hit DyldNeedsEnvVariable() on MaybeReexec() which will return false (on systems where we'd "prefer" to have ReexecDisabled be true), hence we'll just be removing the library path from DYLD_INSERT_VARIABLES env var (if it's there).

If that's correct, then can we use the same trick as DyldNeedsEnvVariable for ReexecDisabled? Or am I making these two to be the same issue when they aren't?

Alex, please take a look at this.

IIUC, ReexecDisabled being false on newer systems will just make us hit DyldNeedsEnvVariable() on MaybeReexec() which will return false (on systems where we'd "prefer" to have ReexecDisabled be true), hence we'll just be removing the library path from DYLD_INSERT_VARIABLES env var (if it's there).

If that's correct, then can we use the same trick as DyldNeedsEnvVariable for ReexecDisabled? Or am I making these two to be the same issue when they aren't?

That's right. This is the reason why it's fine to have ReexecDisable() { return false; } on all macOS versions. On older OS versions, we need return false, on newer it doesn't matter.

zaks.anna accepted this revision.Nov 29 2016, 1:36 PM
zaks.anna added a reviewer: zaks.anna.

LGTM

For non-tests, ReexecDisabled is defined to always return false (in sanitizer_mac.cc). So with this patch, the behavior of instrumented tests will match ASan's default behavior, which brings testing setup closer to the default setup.

ReexecDisabled will only ever be returning true when we are running non-instrumented tests. The reason that is needed is that dyld uses the instrumented binary as a clue to find out if the binary is ASanified and if it should launch the process with DYLD_INTERPOSITION. The default implementation relies on this behavior, if it's not present, we need to re-exact.

This revision is now accepted and ready to land.Nov 29 2016, 1:36 PM

If that's correct, then can we use the same trick as DyldNeedsEnvVariable for ReexecDisabled? Or am I making these two to be the same issue when they aren't?

Do you mean to check dyldVersionNumber? That would just duplicate the code. How about I just document this more in a comment and keep it as return false (for instrumented tests on macOS)?

No need to duplicate if you export the function or make it online anyway.

I don't think we should publish DyldNeedsEnvVariable() as public interface of the sanitizer dylibs. And instrumented tests can't call non-public functions.

But having the comment makes sense. I'm ok with this.

I'll just try to describe this better in a comment. Thanks!

This revision was automatically updated to reflect the committed changes.