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)
Differential D136078
Use-after-return sanitizer binary metadata dvyukov on Oct 17 2022, 6:32 AM. Authored by
Details Currently per-function metadata consists of: This adds a new UAR feature and if it's set an additional element:
Diff Detail
Event TimelineComment Actions 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: As far as I understand the stack layout is only known in machine passes via MachineFrameInfo, Comment Actions Unrelated, but looking at the metadata format more closely, I think this can benefit from LEB128-like varlen encoding. Comment Actions 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.
If that's the case, I think the only option is to implement compression in the linker.
Comment Actions PTAL I've named everything consistently as "SanitizeBinaryMetadata",
Comment Actions PTAL
Comment Actions LGTM
Comment Actions Kazu, thanks for taking care of the revert. I've fixed the assert and tested with -DLLVM_ENABLE_ASSERTIONS=On. Comment Actions FTR ./llvm/test/CodeGen/AMDGPU/llc-pipeline.ll also failed as it hardcodes all passes: Comment Actions Also these: LLVM :: CodeGen/X86/O0-pipeline.ll LLVM :: CodeGen/X86/opt-pipeline.ll need to grep for something. Comment Actions moved *.cpp tests to clang/test/ Comment Actions PTAL moved *.cpp tests to clang/test/ 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)}}})); Comment Actions AFAIK Windows bot will break: https://buildkite.com/llvm-project/premerge-checks/builds/124003#0184c872-af5b-47c0-938c-b31ee8808241 Need to restrict test to Linux only. Comment Actions 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?
Comment Actions @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? Comment Actions 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 Not sure why it run on fuchsia at all and the strange failure mode. Comment Actions It's strange we've not encountered this elsewhere - this must be some special (old?) linker. I'll investigate... Comment Actions
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 I'm going to try and see how I can add the SHF_LINK_ORDER attribute to the section output for the dummy variable. Comment Actions 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. Comment Actions SanitizerBinaryMetadata::createZeroSizedObjectInSection creates __dummy_*, but why is it needed? (There is no comment.) Comment Actions // 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. Comment Actions Thanks! I saw the comment associated with createZeroSizedObjectInSection but wasn't so sure if it was only for section based garbage collection. Comment Actions This seems to break tests: http://45.33.8.238/linux/93224/step_12.txt Can you take a look? Comment Actions 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! |
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?