This is an archive of the discontinued LLVM Phabricator instance.

[dwarf][bugfix] Recompute the size of block before getting it's BestForm
AbandonedPublic

Authored by skan on Jul 29 2021, 2:37 AM.

Details

Summary

http://www.dwarfstd.org/doc/DWARF4.pdf

DWARF blocks come in four forms:

  • DW_FORM_block1 : A 1-byte length followed by 0 to 255 contiguous information bytes
  • DW_FORM_block2 : A 2-byte length followed by 0 to 65,535 contiguous information bytes
  • DW_FORM_block4 : A 4-byte length followed by 0 to 4,294,967,295 contiguous information bytes
  • DW_FORM_block : An unsigned LEB128 length followed by the number of bytes specified by the length

In LLVM, the form is selected by function DIEBlock::BestForm(). The size of the block need to be
recomputed before determing the BestForm, and the function DwarfUnit::addBlock(DIE&,Attribute,DIEBlock*)
forgot to do that.

Diff Detail

Event Timeline

skan created this revision.Jul 29 2021, 2:37 AM
skan requested review of this revision.Jul 29 2021, 2:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 29 2021, 2:37 AM
skan updated this revision to Diff 362963.Jul 29 2021, 8:10 PM

Add a LIT test case.

skan added a comment.Jul 29 2021, 8:19 PM

Test case?

This test case is generated by llvm-tool bugpoint while it is a little big.

It's best to test the code in the unittest of class DwarfUnit, but I could not find such tests.

skan updated this revision to Diff 362965.Jul 29 2021, 8:37 PM

Fix the typo in commit message

skan edited the summary of this revision. (Show Details)Jul 29 2021, 8:38 PM

The size of the block need to be recomputed before determing the BestForm .... the test is too big

So if the BestForm is not right, what bug it will cause, can you simplify the test ?

skan added a comment.Jul 29 2021, 9:08 PM

The size of the block need to be recomputed before determing the BestForm .... the test is too big

So if the BestForm is not right, what bug it will cause, can you simplify the test ?

The issue is that if we get the BestForm before recomputing its size, then BestForm will not be accurate.

To be honest, I am not a expert in dwarf format and don't know how to trigger the bug with a smaller LIT test. However, this bugfix is direct and simple, I think we can fix it even w/o test.

The unnit tests for class DwarfUnit are missing in LLVM, so it takes much more work to test this code by unit test and I think someone who are more familiar with dwarf code could add the unit tests in the future.

@dblaikie @xiangzhangllvm How do you think about it?

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
408

I +1 first.
This addBlock is simplify for line 401, and the Block->BestForm decided by Size, I agree with this change.

If you not clear about the test case, PLS not add it,. I'll accept the code change if no others opps.

yeah, that's too long of a test case - but a test case is generally required.

I'd suggest, if possible, reducing something at the source level, rather than the IR level - do you have access to a source reproduction?

skan added a comment.Jul 29 2021, 10:51 PM

yeah, that's too long of a test case - but a test case is generally required.

I'd suggest, if possible, reducing something at the source level, rather than the IR level - do you have access to a source reproduction?

yes, I have. The source file is simple but includes many internal header files. It's even harder to reduce at the source level...

skan added a comment.Jul 29 2021, 11:03 PM

Let me try. I can learn to add a simple unit test for it.

You may use cvise or creduce to reduce the original source file.

Can we make the test by using YAML (by hard-coding the DWARF)?

Can we make the test by using YAML (by hard-coding the DWARF)?

An example of DWARF YAML test is llvm/test/tools/llvm-dwarfdump/X86/stats-scope-bytes-covered.yaml.

Can we make the test by using YAML (by hard-coding the DWARF)?

An example of DWARF YAML test is llvm/test/tools/llvm-dwarfdump/X86/stats-scope-bytes-covered.yaml.

@Higuoxing may be able to assist with this, if he's around - he did a fair bit of the yaml2obj DWARF support.

skan added a comment.Aug 3 2021, 1:29 AM

You may use cvise or creduce to reduce the original source file.

yeah, I am trying to use creduce and waiting for the result.

skan added a comment.Aug 3 2021, 1:35 AM

Thx. Any help to simplify the test or provide a new test would be appreciated.

You may use cvise or creduce to reduce the original source file.

yeah, I am trying to use creduce and waiting for the result.

I don't expect any improvement from using the creduce here since the metadata part is too long. Something like the [0] may help, but it supports reducing of DILocations only at the moment. Also, there is a WIP (inside SONY) for extending the llvm-reduce to support reducing of DI Metadata part (since it doesn't touch it now).

[0] https://github.com/djolertrk/llvm-metadataburn

You may use cvise or creduce to reduce the original source file.

yeah, I am trying to use creduce and waiting for the result.

I don't expect any improvement from using the creduce here since the metadata part is too long. Something like the [0] may help, but it supports reducing of DILocations only at the moment. Also, there is a WIP (inside SONY) for extending the llvm-reduce to support reducing of DI Metadata part (since it doesn't touch it now).

[0] https://github.com/djolertrk/llvm-metadataburn

Yeah, @MaskRay at least was talking about reducing the original source, not IR - hopefully that's what @skan is doing. It makes for rather more legible/justifiable test cases to reduce the original source to get real-world (rather than quirky reduced) IR (though if we ever got a compact enough syntax, I'd be happy with more further reduced IR or hand-written cases, but as it is now it's so verbose that the hand-modifying to make them more dense usually isn't worth the time and for anything sufficiently complex I worry that machine reduced would make such unrealistic examples as to be unhelpful)

Can we make the test by using YAML (by hard-coding the DWARF)?

An example of DWARF YAML test is llvm/test/tools/llvm-dwarfdump/X86/stats-scope-bytes-covered.yaml.

@Higuoxing may be able to assist with this, if he's around - he did a fair bit of the yaml2obj DWARF support.

IMHO, using unittest is more appropriate. CodeGen is not employed in yaml2obj, hence this code path cannot be tested using YAML.

skan added a comment.Mar 16 2022, 1:19 AM

Thanks all! I failed to reduce the test case by creduce. The source code is C++/SYCL and it triggered lots of bugs of creduce when I tried to reduce it.
Also the class DwarfUnit is an interanl class, which does not have a public constructor, which makes unit test hard to write.

Also, the latest llvm-reduce does not help. Maybe I can wait for the improvement of llvm-reduce about debuginfo.

Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2022, 1:19 AM
skan added a comment.Mar 16 2022, 1:50 AM

unit test?

Not so familiar w/ this component so I don't how to write unit test for this case...
You can take over this PR if unittest for this is easy to you @djtodoro :-)

skan abandoned this revision.Feb 22 2023, 11:01 PM

Abandon this b/c I have no enough time to work on it, feel free to take over it if you have interest :-)

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp