This is an archive of the discontinued LLVM Phabricator instance.

dwp check overflow
ClosedPublic

Authored by zhuna8616 on Feb 22 2023, 8:08 AM.

Diff Detail

Event Timeline

zhuna8616 created this revision.Feb 22 2023, 8:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2023, 8:08 AM
zhuna8616 requested review of this revision.Feb 22 2023, 8:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2023, 8:08 AM

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 added inline comments.Feb 22 2023, 9:38 AM
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 added inline comments.Feb 22 2023, 6:41 PM
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.

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 added inline comments.Feb 22 2023, 6:56 PM
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 added inline comments.Feb 22 2023, 7:23 PM
llvm/tools/llvm-dwp/llvm-dwp.cpp
475

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

ayermolo added inline comments.Feb 22 2023, 9:43 PM
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 added inline comments.Mar 5 2023, 3:15 AM
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 added inline comments.Mar 5 2023, 8:30 AM
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 updated this revision to Diff 502582.Mar 6 2023, 4:15 AM

tests added

zhuna8616 updated this revision to Diff 502583.Mar 6 2023, 4:18 AM

no hard code 32

zhuna8616 updated this revision to Diff 502587.Mar 6 2023, 4:25 AM

commit squashed && test files added

zhuna8616 added a comment.EditedMar 6 2023, 4:34 AM

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 updated this revision to Diff 502986.Mar 7 2023, 4:31 AM

overflow error to warning option

ayermolo added inline comments.Mar 7 2023, 10:53 AM
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 added inline comments.Mar 7 2023, 11:02 AM
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 added inline comments.Mar 7 2023, 11:19 AM
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 added inline comments.Mar 9 2023, 4:43 AM
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 updated this revision to Diff 503739.Mar 9 2023, 5:38 AM

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

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 planned changes to this revision.Mar 9 2023, 9:57 PM

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

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.

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 updated this revision to Diff 506526.Mar 20 2023, 3:42 AM
This comment was removed by zhuna8616.
zhuna8616 updated this revision to Diff 506527.Mar 20 2023, 3:43 AM

Also updated here after rebase.

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 added inline comments.Mar 27 2023, 11:40 AM
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;

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

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 updated this revision to Diff 509008.Mar 28 2023, 7:37 AM

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

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 updated this revision to Diff 509011.Mar 28 2023, 7:47 AM

removed even more sensitive info

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 updated this revision to Diff 509185.Mar 28 2023, 6:52 PM

diff again master

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 added inline comments.Apr 2 2023, 8:30 AM
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 added inline comments.Apr 3 2023, 10:39 AM
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 added inline comments.Apr 3 2023, 10:42 AM
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 updated this revision to Diff 510669.Apr 3 2023, 7:47 PM
  • fix tabs & static_assert & added comments to tests
steven.zhang accepted this revision.EditedApr 4 2023, 12:26 AM

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.

This revision is now accepted and ready to land.Apr 4 2023, 12:26 AM
zhuna8616 updated this revision to Diff 510750.Apr 4 2023, 5:03 AM

format & remove minor changes

  • remove -c
  • format
zhuna8616 requested review of this revision.Apr 10 2023, 4:56 AM
ayermolo accepted this revision.Apr 25 2023, 1:20 PM

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

This revision is now accepted and ready to land.Apr 25 2023, 1:20 PM

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.

zhuna8616 updated this revision to Diff 517561.Apr 27 2023, 8:02 AM
  • format
  • move ContinueOnCuIndexOverflow option decl
dblaikie added inline comments.May 1 2023, 4:39 PM
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)

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

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

dblaikie accepted this revision.May 24 2023, 11:22 AM

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 added a subscriber: lizhengxian.123.
  • rename overflow tests with manual suffix
  • rename overflow tests with manual suffix

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

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

This comment was removed by zhuna8616.

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

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?

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 updated this revision to Diff 526354.May 28 2023, 6:42 PM
  • format squashed

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

I will commit it for you.