This is an archive of the discontinued LLVM Phabricator instance.

Use-after-return sanitizer binary metadata
ClosedPublic

Authored by dvyukov on Oct 17 2022, 6:32 AM.

Details

Summary

Currently per-function metadata consists of:
(start-pc, size, features)

This adds a new UAR feature and if it's set an additional element:
(start-pc, size, features, stack-args-size)

Diff Detail

Event Timeline

dvyukov created this revision.Oct 17 2022, 6:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 17 2022, 6:32 AM
Herald added a subscriber: ormris. · View Herald Transcript
dvyukov requested review of this revision.Oct 17 2022, 6:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 17 2022, 6:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2022, 8:28 AM
dvyukov edited the summary of this revision. (Show Details)Oct 19 2022, 1:54 AM
dvyukov added a reviewer: melver.

Marco, does this look reasonable? Any high-level comments?

There are lots of plumbing to add a new flag and integrate a new pass, so you can look only at:
llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
llvm/lib/CodeGen/SanitizerMetadata.cpp

As far as I understand the stack layout is only known in machine passes via MachineFrameInfo,
so I had to add a machine pass.

Unrelated, but looking at the metadata format more closely, I think this can benefit from LEB128-like varlen encoding.
Function size is small.
Features are very small.
Stack args size is small.
Function start (if we encode it from the end of the previous one) is very small as well.
So potentially it can reduce the entry size from 16 bytes to just 4.
I am just thinking if we use 10x more metadata in future, the size can grow from percents of binary to tens of percents.

dvyukov updated this revision to Diff 468876.Oct 19 2022, 5:18 AM

take args alignment into account

Unrelated, but looking at the metadata format more closely, I think this can benefit from LEB128-like varlen encoding.
Function size is small.
Features are very small.
Stack args size is small.
Function start (if we encode it from the end of the previous one) is very small as well.

Function start is currently encoded as a relative relocation (see AsmPrinter). Anything else, including attempting to use a previous function does not work because of section merging, potential discarding of functions by linker (ffunction-sections), etc.

We could implement compression in the linker, which would be the most reliable. Otherwise we can only play with encoding of the aux constants.

So potentially it can reduce the entry size from 16 bytes to just 4.
I am just thinking if we use 10x more metadata in future, the size can grow from percents of binary to tens of percents.

If that's the case, I think the only option is to implement compression in the linker.

llvm/include/llvm/Transforms/Instrumentation.h
166

inline constexpr

But overall, I'd prefer if these wouldn't need to be defined here, but instead in SanitizerBinaryMetadata.h.

Also, re naming "SanitizerMetadata" is already overloaded and we need to stick to SanitizerBinaryMetadata :-/

llvm/lib/CodeGen/SanitizerMetadata.cpp
1

SanitizerMetadata is already overloaded for other purposes (see clang/lib/CodeGen/SanitizerMetadata.cpp).

(Which is why I had to choose SanitizerBinaryMetadata)

47

I think you also need to check the target section name matches (in operand 0). Because having !pcsections does not imply SanitizerMetadata enabled.

Perhaps at this point it might make sense to move MetadataInfo to SanitizerBinaryMetadata.h (as SanitizerBinaryMetadataInfo perhaps ... long I know :-/ .. unfortunately "SanitizerMetadata" is already overloaded elsewhere -- SanitizerBinaryMDInfo?).

49

APInt

llvm/lib/IR/MDBuilder.cpp
176

This is not great. We still want MDNode interning to work for these.

Doesn't replaceOperandWith() work where you want to replace the operand?

dvyukov updated this revision to Diff 478168.Nov 28 2022, 2:40 AM
dvyukov marked 3 inline comments as done.

rebase and adressed comments

dvyukov marked 2 inline comments as done.Nov 28 2022, 3:11 AM

PTAL

I've named everything consistently as "SanitizeBinaryMetadata",
fixed interning of metadata, and added a test.

dvyukov retitled this revision from [RFC] Use-after-return binary metadata. to Use-after-return sanitizer binary metadata.Nov 28 2022, 4:28 AM
dvyukov added inline comments.Nov 28 2022, 5:02 AM
llvm/lib/CodeGen/SanitizerBinaryMetadata.cpp
79 ↗(On Diff #478168)

Should this be a new method on MDBuilder?

melver added inline comments.Nov 28 2022, 8:54 AM
llvm/include/llvm/Transforms/Instrumentation/SanitizerBinaryMetadata.h
38 ↗(On Diff #478168)

inline constexpr char[]

llvm/lib/CodeGen/SanitizerBinaryMetadata.cpp
79 ↗(On Diff #478168)

As discussed, this could just use the MDBuilder, and then extract the operand out of the new MDNode without using the full MDNode.

llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
242

I think this is no longer PerInstrFeatureMask, but should instead become FeatureMask.

245

Comment why also || Options.UAR (because PerInstrFeatureMask setting of UAR bit is deferred?).

252

What if Options.Covered==true?

Will it still emit some UAR metadata or will it emit something it shouldn't?

Should the F.isVarArg() check be done above in if (PerInstrFeatureMask || (Options.UAR && !F.isVarArg()) ? Then you wouldn't need the PerInstrFeatureMask && RequiresCovered change below and it could still just check RequiresCovered as before.

melver added inline comments.Nov 29 2022, 4:29 AM
llvm/test/Instrumentation/SanitizerBinaryMetadata/uar.cpp
1 ↗(On Diff #478168)

For LLVM IR tests, consider if utils/update_test_checks.py and for MIR utils/update_mir_test_checks.py could help.

Ideally, there'd be no .cpp runtime test, but I also think writing the .ll and .mir tests manually is impossible.

dvyukov updated this revision to Diff 478569.Nov 29 2022, 6:41 AM
dvyukov marked 4 inline comments as done.

addressed comments

dvyukov marked an inline comment as done.Nov 29 2022, 6:47 AM

PTAL

llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
252

What if Options.Covered==true?
Will it still emit some UAR metadata or will it emit something it shouldn't?

If Options.Covered==true && F.isVarArg(), we emit covered metadata with features=0. This looks WAI.

Should the F.isVarArg() check be done above in if (PerInstrFeatureMask || (Options.UAR && !F.isVarArg()) ? Then you wouldn't need the PerInstrFeatureMask && RequiresCovered change below and it could still just check RequiresCovered as before.

It's tricky.
I forgot why I structured the code this way, but I added a new test for all permutations of covered/atomics/uar and it shows the following breakage with your proposed change:

- CHECK-AU: ellipsis: features=1 stack_args=0
+ CHECK-AU: ellipsis: features=3 stack_args=0
llvm/test/Instrumentation/SanitizerBinaryMetadata/covered.cpp
1 ↗(On Diff #478569)

This is the new test.

melver accepted this revision.Nov 29 2022, 7:07 AM

LGTM

llvm/include/llvm/CodeGen/MachinePassRegistry.def
205

I called the LLVM IR pass just "sanmd-module".
So this could just be "machine-sanmd".

llvm/test/Instrumentation/SanitizerBinaryMetadata/common.h
54 ↗(On Diff #478569)

Could make this have its real signature, i.e. add version,start,end args. Then could also check in del that start,end matches, like above.

llvm/test/Instrumentation/SanitizerBinaryMetadata/covered.cpp
1 ↗(On Diff #478569)

Nice!

This revision is now accepted and ready to land.Nov 29 2022, 7:07 AM
melver added inline comments.Nov 29 2022, 7:12 AM
llvm/test/Instrumentation/SanitizerBinaryMetadata/uar.cpp
19 ↗(On Diff #478569)

Is this comment out of place?

dvyukov updated this revision to Diff 478609.Nov 29 2022, 8:08 AM
dvyukov marked 2 inline comments as done.

addressed comments

dvyukov marked an inline comment as done.Nov 29 2022, 8:08 AM
dvyukov added inline comments.
llvm/test/Instrumentation/SanitizerBinaryMetadata/uar.cpp
19 ↗(On Diff #478569)

If a function is completely empty, it won't be marked with UAR feature metadata.
This is super-primitive approximation of the actual analysis we should do (the function has addr-taken/escaped locals/arguments).
So I needed at least somebody for the function.

melver accepted this revision.Nov 29 2022, 8:12 AM
melver added inline comments.
llvm/test/Instrumentation/SanitizerBinaryMetadata/uar.cpp
19 ↗(On Diff #478569)

Ack

This revision was landed with ongoing or failed builds.Nov 29 2022, 8:37 AM
This revision was automatically updated to reflect the committed changes.
melver added inline comments.Nov 29 2022, 9:09 AM
llvm/lib/CodeGen/SanitizerBinaryMetadata.cpp
57 ↗(On Diff #478615)

Probably should be getNumOperands()?

https://reviews.llvm.org/rGdbb11309665fdf7d411d25cc8f2e6a0c8f658143

-DLLVM_ENABLE_ASSERTIONS=On will catch it.

dvyukov reopened this revision.Nov 30 2022, 12:01 AM
This revision is now accepted and ready to land.Nov 30 2022, 12:01 AM
dvyukov updated this revision to Diff 478826.Nov 30 2022, 12:01 AM

fixed the debug build

This revision was landed with ongoing or failed builds.Nov 30 2022, 12:14 AM
This revision was automatically updated to reflect the committed changes.
dvyukov added a subscriber: kazu.Nov 30 2022, 12:15 AM

Kazu, thanks for taking care of the revert.

I've fixed the assert and tested with -DLLVM_ENABLE_ASSERTIONS=On.

dvyukov reopened this revision.Nov 30 2022, 12:39 AM
This revision is now accepted and ready to land.Nov 30 2022, 12:39 AM

FTR ./llvm/test/CodeGen/AMDGPU/llc-pipeline.ll also failed as it hardcodes all passes:
https://lab.llvm.org/buildbot/#/builders/123/builds/14397

FTR ./llvm/test/CodeGen/AMDGPU/llc-pipeline.ll also failed as it hardcodes all passes:
https://lab.llvm.org/buildbot/#/builders/123/builds/14397

Also these:

LLVM :: CodeGen/X86/O0-pipeline.ll
LLVM :: CodeGen/X86/opt-pipeline.ll

need to grep for something.

dvyukov updated this revision to Diff 478887.Nov 30 2022, 4:09 AM

moved *.cpp tests to clang/test/
updated tests that hardcode pipeline passes
fixed metadata update

PTAL

moved *.cpp tests to clang/test/
updated tests that hardcode pipeline passes
fixed metadata update

This did not work, it also changed all shared instances:

const auto *NewMD = MDB.createPCSections(
    {{Section.getString(), {Features, IRB.getInt32(Size)}}});
MD->replaceOperandWith(1, NewMD->getOperand(1));

so I changed it to:

F.setMetadata(LLVMContext::MD_pcsections,
              MDB.createPCSections(
                  {{Section.getString(), {Features, IRB.getInt32(Size)}}}));
melver requested changes to this revision.Nov 30 2022, 5:21 AM
This revision now requires changes to proceed.Nov 30 2022, 5:21 AM
dvyukov updated this revision to Diff 478912.Nov 30 2022, 5:28 AM

restrict tests to linux

melver accepted this revision.Nov 30 2022, 5:47 AM
This revision is now accepted and ready to land.Nov 30 2022, 5:47 AM
This revision was landed with ongoing or failed builds.Nov 30 2022, 5:50 AM
This revision was automatically updated to reflect the committed changes.

Hi, it looks like this change is causing Fuchsia's linux-x64 clang builders to fail: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8796062278266465473/overview

Would you mind taking a look and fixing forward, or that ends up being difficult, reverting until this can be fixed?

aeubanks added inline comments.
clang/test/Instrumentation/SanitizerBinaryMetadata/common.h
1–3 ↗(On Diff #478922)

tests in clang/test cannot assume that standard headers are available, the tests should be hermetic. could you revert and reland with a proper test fix?

covered.cpp and uar.cpp seem to fail on ubuntu bots here.

dvyukov reopened this revision.Dec 1 2022, 8:34 AM
This revision is now accepted and ready to land.Dec 1 2022, 8:34 AM

@melver Re this failure:

******************** TEST 'Clang :: Instrumentation/SanitizerBinaryMetadata/uar.cpp' FAILED ********************
Script:
--
: 'RUN: at line 4';   /home/buildbot/as-worker-91/clang-with-lto-ubuntu/build/stage1/bin/clang --driver-mode=g++ /home/buildbot/as-worker-91/clang-with-lto-ubuntu/llvm-project/clang/test/Instrumentation/SanitizerBinaryMetadata/uar.cpp -o /home/buildbot/as-worker-91/clang-with-lto-ubuntu/build/stage1/tools/clang/test/Instrumentation/SanitizerBinaryMetadata/Output/uar.cpp.tmp -fexperimental-sanitize-metadata=covered,uar && /home/buildbot/as-worker-91/clang-with-lto-ubuntu/build/stage1/tools/clang/test/Instrumentation/SanitizerBinaryMetadata/Output/uar.cpp.tmp | /home/buildbot/as-worker-91/clang-with-lto-ubuntu/build/stage1/bin/FileCheck /home/buildbot/as-worker-91/clang-with-lto-ubuntu/llvm-project/clang/test/Instrumentation/SanitizerBinaryMetadata/uar.cpp
--
Exit Code: 1
Command Output (stderr):
--
/usr/local/bin/ld: sanmd_covered has both ordered [`sanmd_covered[_Z7consumeIjET_RPKcS2_]' in /tmp/lit-tmp-2jisr97l/uar-d5d8d4.o] and unordered [`sanmd_covered[__dummy_sanmd_covered]' in /tmp/lit-tmp-2jisr97l/uar-d5d8d4.o] sections
/usr/local/bin/ld: final link failed: Bad value
clang-16: error: linker command failed with exit code 1 (use -v to see invocation)

https://lab.llvm.org/buildbot/#/builders/124/builds/5759/steps/7/logs/stdio

This looks like a latent issue which is just exposed by the test.

As far as I understand this happens because this dummy variable somehow ends up in an "unordered" section, while other "real" metadata objects end up in "ordered" sections:

void SanitizerBinaryMetadata::createZeroSizedObjectInSection(
    Type *Ty, StringRef SectionSuffix) {
  auto *DummyInit = ConstantAggregateZero::get(ArrayType::get(Ty, 0));
  auto *DummyEntry = new GlobalVariable(Mod, DummyInit->getType(), true,
                                        GlobalVariable::ExternalLinkage,
                                        DummyInit, "__dummy_" + SectionSuffix);
  DummyEntry->setSection(getSectionName(SectionSuffix));
  DummyEntry->setVisibility(GlobalValue::HiddenVisibility);
  if (TargetTriple.supportsCOMDAT())
    DummyEntry->setComdat(Mod.getOrInsertComdat(DummyEntry->getName()));
  // Make sure the section isn't discarded by gc-sections.
  appendToUsed(Mod, DummyEntry);
}

I don't see any method on GlobalVariable to set the section to "ordered".

The only idea I have is to print dummy object in AsmPrinter the same way we print all other metadata objects (then I assume it will have the same properties as other objects).

Do you have any other ideas on how to dead with this?

Re this failure:

project/clang/test/Instrumentation/SanitizerBinaryMetadata/covered.cpp
--
Exit Code: 2

Command Output (stderr):
--
covered.cpp.tmp: /b/s/w/ir/x/w/llvm-llvm-project/clang/test/Instrumentation/SanitizerBinaryMetadata/common.h:22: T consume(const char *&, const char *) [T = unsigned int]: Assertion `pos <= end' failed.
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /b/s/w/ir/x/w/staging/llvm_build/bin/FileCheck -check-prefix=CHECK-A /b/s/w/ir/x/w/llvm-llvm-project/clang/test/Instrumentation/SanitizerBinaryMetadata/covered.cpp

https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket/8796062278266465473/+/u/clang/test/stdout

Not sure why it run on fuchsia at all and the strange failure mode.
But I hope this will go away when we move tests to compiler-rt.

melver added a comment.Dec 1 2022, 9:35 AM

@melver Re this failure:

******************** TEST 'Clang :: Instrumentation/SanitizerBinaryMetadata/uar.cpp' FAILED ********************
Script:
--
: 'RUN: at line 4';   /home/buildbot/as-worker-91/clang-with-lto-ubuntu/build/stage1/bin/clang --driver-mode=g++ /home/buildbot/as-worker-91/clang-with-lto-ubuntu/llvm-project/clang/test/Instrumentation/SanitizerBinaryMetadata/uar.cpp -o /home/buildbot/as-worker-91/clang-with-lto-ubuntu/build/stage1/tools/clang/test/Instrumentation/SanitizerBinaryMetadata/Output/uar.cpp.tmp -fexperimental-sanitize-metadata=covered,uar && /home/buildbot/as-worker-91/clang-with-lto-ubuntu/build/stage1/tools/clang/test/Instrumentation/SanitizerBinaryMetadata/Output/uar.cpp.tmp | /home/buildbot/as-worker-91/clang-with-lto-ubuntu/build/stage1/bin/FileCheck /home/buildbot/as-worker-91/clang-with-lto-ubuntu/llvm-project/clang/test/Instrumentation/SanitizerBinaryMetadata/uar.cpp
--
Exit Code: 1
Command Output (stderr):
--
/usr/local/bin/ld: sanmd_covered has both ordered [`sanmd_covered[_Z7consumeIjET_RPKcS2_]' in /tmp/lit-tmp-2jisr97l/uar-d5d8d4.o] and unordered [`sanmd_covered[__dummy_sanmd_covered]' in /tmp/lit-tmp-2jisr97l/uar-d5d8d4.o] sections
/usr/local/bin/ld: final link failed: Bad value
clang-16: error: linker command failed with exit code 1 (use -v to see invocation)

https://lab.llvm.org/buildbot/#/builders/124/builds/5759/steps/7/logs/stdio

This looks like a latent issue which is just exposed by the test.

As far as I understand this happens because this dummy variable somehow ends up in an "unordered" section, while other "real" metadata objects end up in "ordered" sections:

void SanitizerBinaryMetadata::createZeroSizedObjectInSection(
    Type *Ty, StringRef SectionSuffix) {
  auto *DummyInit = ConstantAggregateZero::get(ArrayType::get(Ty, 0));
  auto *DummyEntry = new GlobalVariable(Mod, DummyInit->getType(), true,
                                        GlobalVariable::ExternalLinkage,
                                        DummyInit, "__dummy_" + SectionSuffix);
  DummyEntry->setSection(getSectionName(SectionSuffix));
  DummyEntry->setVisibility(GlobalValue::HiddenVisibility);
  if (TargetTriple.supportsCOMDAT())
    DummyEntry->setComdat(Mod.getOrInsertComdat(DummyEntry->getName()));
  // Make sure the section isn't discarded by gc-sections.
  appendToUsed(Mod, DummyEntry);
}

I don't see any method on GlobalVariable to set the section to "ordered".

The only idea I have is to print dummy object in AsmPrinter the same way we print all other metadata objects (then I assume it will have the same properties as other objects).

Do you have any other ideas on how to dead with this?

It's strange we've not encountered this elsewhere - this must be some special (old?) linker. I'll investigate...

melver added a comment.EditedDec 2 2022, 2:58 AM

Do you have any other ideas on how to dead with this?

It's strange we've not encountered this elsewhere - this must be some special (old?) linker. I'll investigate...

GNU ld <= 2.35 does not support mixed SHF_LINK_ORDER and non-SHF_LINK_ORDER sections.

This is about the relative order of the section w.r.t. other sections it references (we link it to the function section, see MCObjectFileInfo::getPCSection) when the linker merges the sections. From what I can find, we need to retain SHF_LINK_ORDER to support gc-sections properly.

This is a good summary: https://maskray.me/blog/2021-01-31-metadata-sections-comdat-and-shf-link-order
@MaskRay

I'm going to try and see how I can add the SHF_LINK_ORDER attribute to the section output for the dummy variable.

Do you have any other ideas on how to dead with this?

It's strange we've not encountered this elsewhere - this must be some special (old?) linker. I'll investigate...

GNU ld <= 2.35 does not support mixed SHF_LINK_ORDER and non-SHF_LINK_ORDER sections.

This is about the relative order of the section w.r.t. other sections it references (we link it to the function section, see MCObjectFileInfo::getPCSection) when the linker merges the sections. From what I can find, we need to retain SHF_LINK_ORDER to support gc-sections properly.

This is a good summary: https://maskray.me/blog/2021-01-31-metadata-sections-comdat-and-shf-link-order
@MaskRay

I'm going to try and see how I can add the SHF_LINK_ORDER attribute to the section output for the dummy variable.

This should fix it: https://reviews.llvm.org/D139224 - at least whatever I was able to reproduce with the linker version I found (2.31) seems to work.

SanitizerBinaryMetadata::createZeroSizedObjectInSection creates __dummy_*, but why is it needed? (There is no comment.)

melver added a comment.EditedDec 4 2022, 12:36 AM

SanitizerBinaryMetadata::createZeroSizedObjectInSection creates __dummy_*, but why is it needed? (There is no comment.)

// Create a 0-sized object in a section, so that the section is not discarded
// if all inputs have been discarded.

We need the __begin and __end markers even if there ends up being no binary metadata.

SanitizerBinaryMetadata::createZeroSizedObjectInSection creates __dummy_*, but why is it needed? (There is no comment.)

// Create a 0-sized object in a section, so that the section is not discarded
// if all inputs have been discarded.

We need the __begin and __end markers even if there ends up being no binary metadata.

Thanks! I saw the comment associated with createZeroSizedObjectInSection but wasn't so sure if it was only for section based garbage collection.
If that's the case, D139276 ExternalWeakLinkage would be a more size efficient fix.

dvyukov updated this revision to Diff 480051.Dec 5 2022, 4:35 AM

Moved tests to compiler-rt and rebased to HEAD.

Herald added a project: Restricted Project. · View Herald TranscriptDec 5 2022, 4:35 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
melver accepted this revision.Dec 5 2022, 5:19 AM
This revision was landed with ongoing or failed builds.Dec 5 2022, 5:40 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Dec 5 2022, 6:02 AM

This seems to break tests: http://45.33.8.238/linux/93224/step_12.txt

Can you take a look?

This seems to break tests: http://45.33.8.238/linux/93224/step_12.txt

Can you take a look?

Looking.

Fix for:

-- Supported architectures for crt: aarch64
CMake Error at compiler-rt/cmake/config-ix.cmake:244 (message):
  Unsupported architecture: x86_64
Call Stack (most recent call first):
  compiler-rt/cmake/config-ix.cmake:280 (get_target_flags_for_arch)
  compiler-rt/test/metadata/CMakeLists.txt:7 (get_test_cc_for_arch)
-- Configuring incomplete, errors occurred!

https://reviews.llvm.org/D139323

This seems to break tests: http://45.33.8.238/linux/93224/step_12.txt

Can you take a look?

Sent a fix: https://reviews.llvm.org/D139325