dwp check overflow

[zhuna8616]

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.

[ayermolo]

llvm/tools/llvm-dwp/llvm-dwp.cpp

475

Please make sure this doesn't break https://reviews.llvm.org/D137882
It relies on overflow behavior.

[zhuna8616]

llvm/tools/llvm-dwp/llvm-dwp.cpp

475

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.

[steven.zhang]

I didn't see any tests in the PR. Please add one.

llvm/tools/llvm-dwp/llvm-dwp.cpp

471–472

("Section size overflow in " + SectionPair->first()).str() ?

[steven.zhang]

llvm/tools/llvm-dwp/llvm-dwp.cpp

465

please don't hard code the 32 here. Maybe, you can use sizeof(ContributionOffsets[Index]) or other api that APInt have.

[zhuna8616]

llvm/tools/llvm-dwp/llvm-dwp.cpp

475

I tried it on your test case in https://reviews.llvm.org/D137882 and passed.

[ayermolo]

llvm/tools/llvm-dwp/llvm-dwp.cpp

475

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.
There is a downstream option for us that changes error for previous check into warning. Forgot I didn't upstream it.
https://github.com/llvm/llvm-project/blob/265ea1c7459da95b7a1d30718ac9f970a3ecc8d1/llvm/lib/DWP/DWP.cpp#L672
(which probably should be removed if check is added here).
Actually now that all the changes have landed in llvm and lldb, maybe there should be an option to turn it into an warning? At least for .debug_info part.

[zhuna8616]

llvm/tools/llvm-dwp/llvm-dwp.cpp

475

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?

[ayermolo]

llvm/tools/llvm-dwp/llvm-dwp.cpp

475

No. I am proposing adding an option that when it's turned on this becomes a warning, and overflow behavior is preserved.
Also since this patch also checks .debug_info overflow, the other check can be removed.

[zhuna8616]

tests added

[zhuna8616]

no hard code 32

[zhuna8616]

commit squashed && test files added

[zhuna8616]

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.

[zhuna8616]

overflow error to warning option

[ayermolo]

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

464–465

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);
}

[ayermolo]

llvm/tools/llvm-dwp/llvm-dwp.cpp

464–465

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);
}

464–465

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);
}

errr, messeed up APIs. never mind.

[ayermolo]

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."),

?

[zhuna8616]

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?

[zhuna8616]

move warning option to dwp dayer && add check for invalid sequence

[ayermolo]

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
At least locally. The test in comments uses: .zero 0xfffffff0 - 0x2b # 0xfffffff0 is mimimum reserved length
Which actually creates a 4GB file on disk. Don't think we want that in build bots.

[zhuna8616]

We intend this for llvm release/11.x version. Perhaps this can go as a backport?

[ayermolo]

In D144565#4183779, @zhuna8616 wrote:

We intend this for llvm release/11.x version. Perhaps this can go as a backport?

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.

[steven.zhang]

In D144565#4183779, @zhuna8616 wrote:

We intend this for llvm release/11.x version. Perhaps this can go as a backport?

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)

[zhuna8616]

I made another patch https://reviews.llvm.org/D146289

[zhuna8616]

This comment was removed by zhuna8616.

[zhuna8616]

Also updated here after rebase.

[steven.zhang]

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 ?

[dblaikie]

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;

[ayermolo]

To clarify https://reviews.llvm.org/D146289 will be going into the trunk right?

[ayermolo]

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]
Just to keep it consistent with other checks.

llvm/test/tools/llvm-dwp/X86/overflow_debug_info.test

2 ↗

(On Diff #506527)

Actually looking at assembly those are DWARF4 tests.
example:

.short	4                               # DWARF version number

.byte 4 # DW_AT_GNU_dwo_name
.quad -346972125991005518 # DW_AT_GNU_dwo_id

[zhuna8616]

fix comments & added dwarf v4 and v5 checks & removed sensitive info

[zhuna8616]

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

[zhuna8616]

removed even more sensitive info

[zhuna8616]

In D144565#4225265, @ayermolo wrote:

To clarify https://reviews.llvm.org/D146289 will be going into the trunk right?

This post is rebased to main so D146289 is obsolete. This one can goto trunk.

[zhuna8616]

diff again master

[ayermolo]

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?

[zhuna8616]

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?

[ayermolo]

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.

[ayermolo]

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.
static_assert(sizeof(OldOffset) == sizeof(TypesOffset)));

[zhuna8616]

• fix tabs & static_assert & added comments to tests

[steven.zhang]

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.

[zhuna8616]

format & remove minor changes

• remove -c
• format

[zhuna8616]

gentle ping

[zhuna8616]

gentle ping @ayermolo

[ayermolo]

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? :)

[ayermolo]

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

[dblaikie]

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.

[zhuna8616]

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.

[zhuna8616]

• format
• move ContinueOnCuIndexOverflow option decl

[dblaikie]

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)

[zhuna8616]

Perhaps @ayermolo can shed some light on a precedent case.

[ayermolo]

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.

[dblaikie]

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.

[zhuna8616]

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?

[steven.zhang]

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

[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

[lizhengxian.123]

• rename overflow tests with manual suffix

[zhuna8616]

• rename overflow tests with manual suffix

[zhuna8616]

Sorry about the name hazard... @lizhengxian.123 this guy's git config somehow overwrote everyone's.

[zhuna8616]

I do not have commit access. Perhaps someone else should commit this.

[zhuna8616]

This comment was removed by zhuna8616.

[ayermolo]

Looks like pre-merge check clang-format is failing.

[zhuna8616]

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?

[ayermolo]

In D144565#4377842, @zhuna8616 wrote:

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?

try git-clang-format --commit HEAD~

[zhuna8616]

• format

[zhuna8616]

• format squashed

[zhuna8616]

Now that there is no problem. Perhaps we can finally commit?

[steven.zhang]

I will commit it for you.

llvm/tools/llvm-dwp/llvm-dwp.cpp

static Error handleSection(
 
   auto SectionPair = KnownSections.find(Name);
   if (SectionPair == KnownSections.end())
     return Error::success();
 
   if (DWARFSectionKind Kind = SectionPair->second.second) {
     auto Index = getContributionIndex(Kind);
     if (Kind != DW_SECT_EXT_TYPES) {
       CurEntry.Contributions[Index].Offset = ContributionOffsets[Index];
-      ContributionOffsets[Index] +=
-          (CurEntry.Contributions[Index].Length = Contents.size());
+      APInt Offset{32, ContributionOffsets[Index], false};
+      APInt NewLength{
+          32, CurEntry.Contributions[Index].Length = Contents.size(), false};
+      bool Overflow = false;
+      APInt NewOffset = Offset.uadd_ov(NewLength, Overflow);
+      if (Overflow) {
+        std::string SectionName = SectionPair->first().str();
+        return make_error<DWPError>(
+            std::string("Section size overflow in " + SectionName));
+      }
+      ContributionOffsets[Index] = NewOffset.getLimitedValue();
     }
 
     switch (Kind) {
     case DW_SECT_INFO:
       InfoSection = Contents;
       break;
     case DW_SECT_ABBREV:
       AbbrevSection = Contents;