This is an archive of the discontinued LLVM Phabricator instance.

[part 1] sancov/inline-bool-flag instrumentation.
ClosedPublic

Authored by pratyai on Apr 1 2020, 2:18 PM.

Details

Summary

New SanitizerCoverage feature inline-bool-flag which inserts an atomic store of 1 to a boolean (which is an 8bit integer in practice) flag on every instrumented edge.

Implementation-wise it's very similar to inline-8bit-counters features. So, much of wiring and test just follows the same pattern.

Tested with cmake --build . -- check-llvm.

Diff Detail

Event Timeline

pratyai created this revision.Apr 1 2020, 2:18 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 1 2020, 2:18 PM
Herald added subscribers: Restricted Project, cfe-commits, jfb, hiraditya. · View Herald Transcript
pratyai updated this revision to Diff 254312.Apr 1 2020, 2:36 PM

Removed some unintentional diffs.

Removed some unintentional diffs from clang/lib/Frontend/CompilerInvocation.cpp.

pratyai updated this revision to Diff 254316.Apr 1 2020, 2:44 PM

Still had a couple of lines of diff left :(

LGTM
would it be possible to split into 3 patches

  1. Instrumentation
  2. clang flags stuff
  3. compiler-rt test

Btw. is this clang formated?

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
484

Int1PtrT?

922

Unordered or even NotAtomic is good enough here

pratyai updated this revision to Diff 254418.Apr 1 2020, 9:47 PM
  • Switched to Int1Ty.
  • Switched Atomic::NonAtomic (same as just dropping the two lines?)
  • C++ files were clang-formatted, but arc lint couldn't find clang-format-diff before. Now it seems to have formatted the lit tests.
  • Will split the patch later (i.e. remove all non-instrumentation stuff from this patch, then move flags & compiler-rt tests into two other patches).
pratyai marked 2 inline comments as done.Apr 1 2020, 9:48 PM
vitalybuka added inline comments.Apr 2 2020, 12:03 AM
clang/test/Driver/autocomplete.c
49 ↗(On Diff #254418)

probably you didn't add "--style=file"
this dir should not enforce line length

pratyai updated this revision to Diff 254461.Apr 2 2020, 2:23 AM

Undid the arc lint on autocomplete.c.
arc lint does not seem to have an option --style, but it's just one line diff anyway.

pratyai updated this revision to Diff 254462.Apr 2 2020, 2:30 AM

Did the same for fsanitize-coverage.c.

pratyai marked an inline comment as done.Apr 2 2020, 2:35 AM
pratyai updated this revision to Diff 254468.Apr 2 2020, 2:55 AM

fsanitize-coverage.c undo was incorrect the first time :(

pratyai updated this revision to Diff 254470.Apr 2 2020, 3:00 AM

And missed a space.
Sorry, I'm just not very familiar with this kind of test, and cannot spot the mistakes before going to the web UI.

It looks like I broke the tests after the i8 -> i1 switch.

I think it's because of an existing bug. From https://llvm.org/docs/LangRef.html

i1:8:8 - i1 is 8-bit (byte) aligned

OTOH, in SanitizerCoverage.cpp, we have in CreateFunctionLocalArrayInSection():

Array->setAlignment(Align(Ty->isPointerTy()
                             ? DL->getPointerSize()
                             : Ty->getPrimitiveSizeInBits() / 8));

IIUC getPrimitiveSizeInBits() / 8 would be 1 / 8 => 0 for i1 type (it works for other int types which have multiple-of-8 bits.

PLMK if my assessment is correct, and if so if I should fix it in a separate patch, or just keep that in here.

I'll leave this patch "un-split" for now.

Thanks!

pratyai updated this revision to Diff 254508.Apr 2 2020, 6:45 AM

Added the alignment for i1 as a special case.

Also the inline-bool-flag.ll test had to be changed with it. PTAL if I am missing something; I did not expect it to be much diffrerent than inline-8bit-counters.ll, but evidently it is now.

pratyai updated this revision to Diff 254509.Apr 2 2020, 6:46 AM

Added the alignment for i1 as a special case.

Also the inline-bool-flag.ll test had to be changed with it. PTAL if I am missing something; I did not expect it to be much diffrerent than inline-8bit-counters.ll, but evidently it is now.

pratyai updated this revision to Diff 254919.EditedApr 3 2020, 2:28 PM
pratyai retitled this revision from sancov/inline-bool-flag feature + tests + docs. to [part 1] sancov/inline-bool-flag instrumentation..

Dropped files from outside llvm/.../Instrumentation* as suggested.
The clang flags & compiler-rt tests will be added in future diffs.

Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2020, 2:28 PM
pratyai updated this revision to Diff 255619.Apr 7 2020, 2:37 AM

Undid the arc lint on autocomplete.c.
arc lint does not seem to have an option --style, but it's just one line diff anyway.

yep. it had --style LLVM by mistake. I guess it's fixed now.

It looks like I broke the tests after the i8 -> i1 switch.

I think it's because of an existing bug. From https://llvm.org/docs/LangRef.html

i1:8:8 - i1 is 8-bit (byte) aligned

OTOH, in SanitizerCoverage.cpp, we have in CreateFunctionLocalArrayInSection():

Array->setAlignment(Align(Ty->isPointerTy()
                             ? DL->getPointerSize()
                             : Ty->getPrimitiveSizeInBits() / 8));

IIUC getPrimitiveSizeInBits() / 8 would be 1 / 8 => 0 for i1 type (it works for other int types which have multiple-of-8 bits.

PLMK if my assessment is correct, and if so if I should fix it in a separate patch, or just keep that in here.

I'll leave this patch "un-split" for now.

Thanks!

Looks like a bug.
Separate patch would be nice. If it goes before this one we can expect NOP.
However, if you don't have committer access, I am going to submit this for you, and I can split the patch myself.

vitalybuka added inline comments.Apr 8 2020, 5:56 PM
llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
922

Store->setAtomic(AtomicOrdering::NotAtomic); should be defaul

923

isn't alignment is always 1 which is already default?

pratyai updated this revision to Diff 256167.Apr 8 2020, 8:30 PM
pratyai marked 2 inline comments as done.

It looks like I broke the tests after the i8 -> i1 switch.

I think it's because of an existing bug. From https://llvm.org/docs/LangRef.html

i1:8:8 - i1 is 8-bit (byte) aligned

OTOH, in SanitizerCoverage.cpp, we have in CreateFunctionLocalArrayInSection():

Array->setAlignment(Align(Ty->isPointerTy()
                             ? DL->getPointerSize()
                             : Ty->getPrimitiveSizeInBits() / 8));

IIUC getPrimitiveSizeInBits() / 8 would be 1 / 8 => 0 for i1 type (it works for other int types which have multiple-of-8 bits.

PLMK if my assessment is correct, and if so if I should fix it in a separate patch, or just keep that in here.

I'll leave this patch "un-split" for now.

Thanks!

Looks like a bug.
Separate patch would be nice. If it goes before this one we can expect NOP.
However, if you don't have committer access, I am going to submit this for you, and I can split the patch myself.

That'd be cool, thanks!

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
922

Right; dropping.

923

Right; the default is NoneType::None, which is already 1. Dropping.

pratyai updated this revision to Diff 256175.Apr 8 2020, 9:00 PM

oops; forgot that dropping the alignment == 1 ? 1 : alignment / 8 bit off this diff wouldn't work with build.

(running a --check-llvm, but it's a bit slow; if things break I'll update again)

pratyai updated this revision to Diff 256182.Apr 8 2020, 9:46 PM

Adjusted the diff in the change (align 1 is dropped from the llvm instruction of the CHECK). The reason is that it does not get set until setAlignment is explicitly called (ref: https://llvm.org/doxygen/Instructions_8cpp_source.html#l01421). From https://llvm.org/docs/LangRef.html#global-variables

If not present, or if the alignment is set to zero, the alignment of the global is set by the target to whatever it feels convenient. If an explicit alignment is specified, the global is forced to have exactly that alignment.

vitalybuka accepted this revision.Apr 8 2020, 9:56 PM
This revision is now accepted and ready to land.Apr 8 2020, 9:56 PM
This revision was automatically updated to reflect the committed changes.