This is an archive of the discontinued LLVM Phabricator instance.

[lit] Support the `%shared_libasan` lit substitution on Apple platforms.
ClosedPublic

Authored by delcypher on Oct 10 2018, 2:43 PM.

Details

Summary

The previous value looks Linux specific so that has been guarded with
the host OS being Linux.

On Apple platforms %shared_libasan expands to the absolute path of the
ASan dylib.

Previously on Linux %shared_libasan expanded to just the file name
of the shared library rather than the absolute path to the library.
This is likely a bug because it would rely on the OS's dynamic linker
to find the shared library which could accidentally pick up a system copy
rather than the shared library that was just built.

For other platforms we emit a warning if config.asan_dynamic is true.

This patch also only defines the substitution when config.asan_dynamic
is true because using this substitution only makes sense when the
dynamic library is available.

Diff Detail

Repository
rL LLVM

Event Timeline

delcypher created this revision.Oct 10 2018, 2:43 PM
Herald added a subscriber: Restricted Project. · View Herald TranscriptOct 10 2018, 2:43 PM

What breaks if we switch to the absolute path on Linux?

test/asan/lit.cfg
108–113 ↗(On Diff #169099)

This is unnecessarily expanded. Fold at least the .format() call into one line (personally I'd have this whole thing on one line).

I'd be fine with using an absolute path on all platforms, it's what the Clang driver does as well so this certainly looks like a bug.

What breaks if we switch to the absolute path on Linux?

I'm not sure. I will test and report back.

test/asan/lit.cfg
108–113 ↗(On Diff #169099)

Sure. I'll fix this.

What breaks if we switch to the absolute path on Linux?

I'm not sure. I will test and report back.

@kubamracek Linux tests seem to pass if I use an absolute path so I'll make the change for Linux too.

delcypher updated this revision to Diff 169346.Oct 11 2018, 6:36 PM
  • Try to fix formatting
  • Use absolute path on Linux too
  • Emit a warning for other platforms when config.asan_dynamic is true.
  • Only set substitution when config.asan_dynamic is true.
delcypher marked 2 inline comments as done.Oct 11 2018, 6:36 PM
phosek accepted this revision.Oct 11 2018, 6:40 PM

LGTM

This revision is now accepted and ready to land.Oct 11 2018, 6:40 PM

LGTM

@phosek Thanks for the review. I'll wait for a LGTM from @kubamracek or @george.karpenkov so they can check the Apple side of this.

delcypher edited the summary of this revision. (Show Details)Oct 11 2018, 7:48 PM
eugenis requested changes to this revision.Oct 12 2018, 9:39 AM
eugenis added a subscriber: eugenis.

I think this might break android which runs tests at a different absolute path.

This revision now requires changes to proceed.Oct 12 2018, 9:39 AM
delcypher added a comment.EditedOct 12 2018, 9:53 AM

I think this might break android which runs tests at a different absolute path.

Hmm now that I think about it, that's very likely. I don't have an Android device I can test with so I can't really investigate this. How about I add an extra condition that checks for config.android and if that's true, use the old behaviour (filename rather than an absolute path) along with a FIXME comment?

eugenis accepted this revision.Oct 12 2018, 10:02 AM

nvm, all tests using this substitution are disabled on Android anyway

This revision is now accepted and ready to land.Oct 12 2018, 10:02 AM
kubamracek accepted this revision.Oct 12 2018, 4:36 PM
This revision was automatically updated to reflect the committed changes.