This is an archive of the discontinued LLVM Phabricator instance.

[SanitizerCoverage][NFC] Use appendToUsed instead of include
ClosedPublic

Authored by metzman on Jan 6 2019, 3:51 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

metzman created this revision.Jan 6 2019, 3:51 PM
metzman updated this revision to Diff 180418.Jan 6 2019, 3:55 PM
  • fix comment
metzman retitled this revision from [SanitizerCoverage] NFC: Use appendToUsed instead of include to [SanitizerCoverage][NFC] Use appendToUsed instead of include.Jan 7 2019, 7:49 AM
metzman added a reviewer: pcc.

Peter could you please take a look.
This change uses appendToUsed instead of /include, for the constructors,. as you suggested.

pcc added a comment.Jan 7 2019, 8:48 AM

Doesn't this need a test case update? (or a new test case, if this code wasn't being covered before)

In D56369#1348358, @pcc wrote:

Doesn't this need a test case update? (or a new test case, if this code wasn't being covered before)

There is a(n indirect) test for this that is general enough that it doesn't need an update.
In libFuzzer there is a test to ensure fuzz targets can be compiled with the "/OPT:REF" linker option.
If "/include" isn't emitted during IR generation this test fails, because the constructors get stripped.
The test still passes though, because as you correctly pointed out in your email to me, appendToUsed causes /include to be emitted.

If you would like I can try to add a more direct test to LLVM to ensure that /include is emitted during IR generation.

pcc added a comment.Jan 9 2019, 10:30 AM

Can you add the test that /include is being emitted?

pcc added a comment.Jan 9 2019, 10:30 AM

Or just test that the ctor appears in llvm.used

metzman updated this revision to Diff 180953.Jan 9 2019, 3:56 PM
  • Add test
metzman updated this revision to Diff 180967.Jan 9 2019, 4:52 PM
  • fix comment
In D56369#1351477, @pcc wrote:

Or just test that the ctor appears in llvm.used

Done.

pcc accepted this revision.Jan 11 2019, 12:45 PM

LGTM

This revision is now accepted and ready to land.Jan 11 2019, 12:45 PM
This revision was automatically updated to reflect the committed changes.

Unrelated to this change, I made an error in my commit message for: r351356
referring to this review instead of https://reviews.llvm.org/D56726. Sorry for any confusion this may cause.