Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
This patch checks whether certain section overflows when generating a dwp file. If overflow occurs, llvm-dwp fails fast and dump info on the subject.
This is supposed to backport to branch release/11.x.
| llvm/tools/llvm-dwp/llvm-dwp.cpp | ||
|---|---|---|
| 475 ↗ | (On Diff #499514) | Please make sure this doesn't break https://reviews.llvm.org/D137882 |
| llvm/tools/llvm-dwp/llvm-dwp.cpp | ||
|---|---|---|
| 475 ↗ | (On Diff #499514) | When nothing overflows, getLimitedValue simply returns the raw data of this APInt. Since there is an overflow check, this is synonyms to something like getValue. |
I didn't see any tests in the PR. Please add one.
| llvm/tools/llvm-dwp/llvm-dwp.cpp | ||
|---|---|---|
| 471–472 ↗ | (On Diff #499514) | ("Section size overflow in " + SectionPair->first()).str() ? |
| llvm/tools/llvm-dwp/llvm-dwp.cpp | ||
|---|---|---|
| 465 ↗ | (On Diff #499514) | please don't hard code the 32 here. Maybe, you can use sizeof(ContributionOffsets[Index]) or other api that APInt have. |
| llvm/tools/llvm-dwp/llvm-dwp.cpp | ||
|---|---|---|
| 475 ↗ | (On Diff #499514) | I tried it on your test case in https://reviews.llvm.org/D137882 and passed.  |
| llvm/tools/llvm-dwp/llvm-dwp.cpp | ||
|---|---|---|
| 475 ↗ | (On Diff #499514) | Sorry wasn't clear. That test doesn't use llvm-dwp, I just pointed to the diff to point out that there is a code in llvm that relies on overflow behavior to reconstruct cu-index. |
| llvm/tools/llvm-dwp/llvm-dwp.cpp | ||
|---|---|---|
| 475 ↗ | (On Diff #499514) | This patch is meant to backport llvm/11.x version, where no overflow check exists. The latest implementation checks overflow in debug_info section only. Am I supposed to turn the error introduced in this patch to a warning? |
| llvm/tools/llvm-dwp/llvm-dwp.cpp | ||
|---|---|---|
| 475 ↗ | (On Diff #499514) | No. I am proposing adding an option that when it's turned on this becomes a warning, and overflow behavior is preserved. |
I added a test case for detecting overflow cases. I will look into the error to warning option presently.
The gist of the test is:
- create 2 simple C code files main.c, hello.c with skeleton functions, one calling the other
- compile them with split-dwarf option on to assembly code
- manually change the section attribute of hello.s .debug_info.dwo, and let the section have size close to 2^32-1
- compile & link the 2 .s files and llvm-dwp the linked result
The last step should produce an error prompt, indicating an overflow.
| llvm/lib/MC/MCTargetOptionsCommandFlags.cpp | ||
|---|---|---|
| 90 ↗ | (On Diff #502986) | I don't see a reason why this should be MC layer option. Can you move it to llvm-dwp? |
| llvm/test/tools/llvm-dwp/X86/overflow-warning.test | ||
| 4 ↗ | (On Diff #502986) | Can you check that overflow behavior is preserved? |
| llvm/tools/llvm-dwp/llvm-dwp.cpp | ||
| 470 ↗ | (On Diff #502986) | I think this will break the overflow behavior that llvm patch I references relies on. APInt APInt::uadd_sat(const APInt &RHS) const {
bool Overflow;
APInt Res = uadd_ov(RHS, Overflow);
if (!Overflow)
return Res;
return APInt::getMaxValue(BitWidth);
} |
| llvm/tools/llvm-dwp/llvm-dwp.cpp | ||
|---|---|---|
| 470 ↗ | (On Diff #502986) |
|
| 470 ↗ | (On Diff #502986) |
errr, messeed up APIs. never mind. |
| llvm/lib/MC/MCTargetOptionsCommandFlags.cpp | ||
|---|---|---|
| 91 ↗ | (On Diff #502986) | For name maybe: static cl::opt<bool> ContinueOnCuIndexOverflow( "continue-on-cu-index-overflow",
cl::desc("This turns an error when offset for .debug sections "
"overfolws into a warning."),? |
| llvm/test/tools/llvm-dwp/X86/overflow-warning.test | ||
|---|---|---|
| 4 ↗ | (On Diff #502986) | Do I check it as in https://reviews.llvm.org/D137882 ? I may have to copy much of the code if I am to check overflow behavior as in this post. I found a invalid_cu_index.test that goes like: RUN: not llvm-dwp %p/../Inputs/invalid_cu_index/x.dwp -o %t 2>&1 | FileCheck %s CHECK: error: failed to parse cu_index Looks like it's parsing cu index with llvm-dwp directly. I added a similar check and it passed. RUN: llvm-dwp overflow.dwp -o overflow-warned.dwp Is this line of code sufficient for verifying overflow behavior? Or should I follow this post https://reviews.llvm.org/D137882 instead? |
You might want to rebase this on trunk LLVM. Seems like you are pushing changes from an old version.
| llvm/test/tools/llvm-dwp/X86/overflow-warning.test | ||
|---|---|---|
| 4 ↗ | (On Diff #502986) | Sorry should have been more clear. With option that turns it in to a warning can you check that for sections that overflow have correct reconstructed offset: # RUN: llvm-dwarfdump --manaully-generate-unit-index --debug-cu-index %t.dwp | FileCheck %s The test in that diff uses zero |
ah my apologies. I thought you were going to push it to trunk, and then backport internally to 11.x. Wrong mental model. :)
TBH I am not sure what the procedure is for pushing just to release branch. Maybe Tom Stellard will know.
Maybe, go into the master first, and then, considering the service line (Patches won't go into those lines except that they are critical bug fixes, as I know)
Some minor comments.
| llvm/lib/DWP/DWP.cpp | ||
|---|---|---|
| 192 ↗ | (On Diff #506527) | Do you format the changes ? |
| 193 ↗ | (On Diff #506527) | I would prefer to use the StringRef to concat them, and then, convert base to std::string. |
| 238 ↗ | (On Diff #506527) | Please remove the {} if there is only one statement in the if clause. The same rule applies in other changes. |
| 728–729 ↗ | (On Diff #506527) | Remove this comments. |
| llvm/test/tools/llvm-dwp/Inputs/overflow/debug_info.s | ||
| 79 ↗ | (On Diff #506527) | remove such kind of information |
| llvm/test/tools/llvm-dwp/X86/overflow_debug_info.test | ||
| 2 ↗ | (On Diff #506527) | How about the dwarf 4 ? |
| llvm/lib/DWP/DWP.cpp | ||
|---|---|---|
| 238 ↗ | (On Diff #506527) | We'd also usually roll in the initialization to the if condition, and prefer = init over () init (avoids explicit conversions - so it's easier to read without worrying about any more "interesting" conversions happening): if (Error Err = sectionOverflowErrorOrWarning(...)) return Err; |
Don't really have a strong opinion, but maybe other reviewers do. Right now we have two types of checks;
pre condition check:
std::numeric_limits<uint32_t>::max() - InfoSectionOffset <
C.getLength32()
and
post condition check:
OldOffset > TypesOffset
Should this be standardized to one or the other type of check?
| llvm/lib/DWP/DWP.cpp | ||
|---|---|---|
| 234 ↗ | (On Diff #506527) | Maybe a static_assert that sizeof(OldOffset) == sizeof(TypeOffset)? |
| 688 ↗ | (On Diff #506527) | OldOffset > ContributionOffsets[Index] |
| llvm/test/tools/llvm-dwp/X86/overflow_debug_info.test | ||
| 2 ↗ | (On Diff #506527) | Actually looking at assembly those are DWARF4 tests. .short 4 # DWARF version number .byte 4 # DW_AT_GNU_dwo_name |
I removed the debug_str section checks because it seemed redundant.
Also made separate tests for dwarf v4 v5.
| llvm/lib/DWP/DWP.cpp | ||
|---|---|---|
| 192 ↗ | (On Diff #506527) | done |
| 193 ↗ | (On Diff #506527) | done |
| 234 ↗ | (On Diff #506527) | done |
| 238 ↗ | (On Diff #506527) | done |
| 688 ↗ | (On Diff #506527) | done |
| 728–729 ↗ | (On Diff #506527) | done |
| llvm/test/tools/llvm-dwp/Inputs/overflow/debug_info.s | ||
| 79 ↗ | (On Diff #506527) | done |
For. your assembly tests. Can you add comment(s) for parts that you have modified to make the overflow happen.
Also maybe add what c/c++ was used to generate it.
For. your tests please add description of what they are doing.
| llvm/lib/DWP/DWP.cpp | ||
|---|---|---|
| 237 ↗ | (On Diff #509185) | I don't think this is the same as sizeof(OldOffset) == sizeof(TypesOffset) |
| 861 ↗ | (On Diff #509185) | Is this because of tabs? |
| llvm/lib/DWP/DWP.cpp | ||
|---|---|---|
| 237 ↗ | (On Diff #509185) | Err... typos... My bad |
| 861 ↗ | (On Diff #509185) | Is this a format issue? Should I make it like this: if (Error Err = addAllTypesFromDWP(Out, TypeIndexEntries, TUIndex, OutSection,
TypeInputSection, CurEntry,
ContributionOffsets[TypesContributionIndex],
TypesContributionIndex))Or should I add a tab to the left of addAllTypesFromDWP? |
| llvm/lib/DWP/DWP.cpp | ||
|---|---|---|
| 861 ↗ | (On Diff #509185) | Just wondering if you have tab enabled in your IDE. From what I remember at least on windows VS it's the default, and you need to change it to 4 spaces. |
| llvm/lib/DWP/DWP.cpp | ||
|---|---|---|
| 237 ↗ | (On Diff #509185) | To be clear I think it's still a good idea to have the static_assert since you are relying on both being same type in your check. |
LGTM now with one minor comment. But please hold on to see if ayermolo has other comments.
| llvm/test/tools/llvm-dwp/Inputs/overflow/main_v4.s | ||
|---|---|---|
| 5 ↗ | (On Diff #510669) | -c here is not needed. |
My apologies, it's been a busy last couple of weeks.
Looks fine to me.
I would add static_assert(sizeof(OldOffset) == sizeof(TypesOffset)); checks for other places where you do similar > comparisons.
@dblaikie do you have any comments? :)
Also looks like one of your pre-merge checks has failed with build error:
ld.lld: error: undefined symbol: DwpCategory
referenced by CommandLine.h:473 (/var/lib/buildkite-agent/builds/llvm-project/llvm/include/llvm/Support/CommandLine.h:473)
DWP.cpp.o:(_GLOBAL__sub_I_DWP.cpp) in archive lib/libLLVMDWP.a
Could this have a more verbose/explicit description? I thought there was already some overflow checking in llvm-dwp - so it'd be good to describe in more detail which cases this is intended to add.
This patch is intended to add overflow checks to all dwo sections. Previous versions check whether debug_info section overflows, ignoring other sections.
This patch also adds tests for overflow cases, verifying that new code does report overflow when it happens.
| llvm/test/tools/llvm-dwp/Inputs/overflow/debug_info_v5.s | ||
|---|---|---|
| 3–4 ↗ | (On Diff #517561) | This is going to create rather large files on test machines, which is a bit concerning... Is there precedent for this/maybe it needs some broader discussion? (or maybe we leave these tests around but with manual repro steps/some way to opt-in to them with an env variable or lit flag or something) |
Well when I added a check I had a local test and posted it as part of a review, but not committed. I think there was the same concern about the size. Although I did it with explict .zero op.
Yeah, I wouldn't mind the tests being included somewhere off by default in some way. I don't think it's feasible to have single tests create 4GB+ of temp/output files.
We have to make huge files, or overflow won't happen. Perhaps we can disable the related test by default in lit, making it run on demand?
I think, this warning is quite important. Without it, we will silently generate a bad dwp, And it will crash the gdb or report something like: invalid dwarf version, which is really confusion and hard to address the root cause. Maybe, we can remove such tests if the size of the test output is the concern. What do you think ? @ayermolo @dblaikie
Yeah. Please submit this without any tests that produce 4GB/large amount sof data on test machines.Either include those tests with some sort of "manual" marking (eg: with "manual" as a suffix so they don't get picked up by the usual lit test globs) or perhaps omitting them entirely (they'll still be here in the code review history). Awkward either way.
If you happen to be up for looking around to see if there's any precedent/other manual tests in LLVM - maybe this patch can be consistent with however those are annotated
Sorry about the name hazard... @lizhengxian.123 this guy's git config somehow overwrote everyone's.
This is weird. I am unable to reproduce clang-format error locally.
These are the steps to reproduce according to https://buildkite.com/llvm-project/premerge-checks/builds/154962#01885cac-1fac-4352-af57-3fc9634f6010.
rm -rf build mkdir build cd build export CC="clang" export CXX="clang++" export LD="LLD" cmake ../llvm -D LLVM_ENABLE_PROJECTS="libc;flang;polly;lld;llvm;clang;bolt;mlir;compiler-rt;clang-tools-extra" -G Ninja -D CMAKE_BUILD_TYPE=Release -D LLVM_ENABLE_ASSERTIONS=ON -D LLVM_BUILD_EXAMPLES=ON -D COMPILER_RT_BUILD_LIBFUZZER=OFF -D LLVM_LIT_ARGS="-v --xunit-xml-output test-results.xml" -D LLVM_ENABLE_LLD=ON -D CMAKE_CXX_FLAGS=-gmlt -DBOLT_CLANG_EXE=/usr/bin/clang # ^note that compiler cache arguments are omitted ln -s $PWD/compile_commands.json ../compile_commands.json ninja check-all check-clang-tools check-polly check-clang check-llvm check-flang check-mlir check-lld check-libc check-bolt cd .. git-clang-format HEAD~1
I executed them and got.
no modified files to format
Is there anything more to it before executing git-clang-format?