-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[DebugInfo] Add dataSize to DIBasicType to add DW_AT_bit_size to _BitInt types #164372
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
83f7c7c
87d5630
efefb69
0531d69
026976e
a0bcf16
965ac41
17f9c92
2ea40d0
6c3cabd
2223b9a
8b43968
251bcac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| // RUN: %clang_cc1 -x c++ %s -debug-info-kind=standalone -gno-column-info -emit-llvm -o - | FileCheck %s | ||
| // RUN: %clang_cc1 -x c %s -debug-info-kind=standalone -gno-column-info -emit-llvm -o - | FileCheck %s | ||
|
|
||
| unsigned _BitInt(17) a; | ||
| _BitInt(2) b; | ||
|
|
||
| // CHECK: !DIBasicType(name: "_BitInt", size: 8, dataSize: 2, encoding: DW_ATE_signed) | ||
| // CHECK: !DIBasicType(name: "unsigned _BitInt", size: 32, dataSize: 17, encoding: DW_ATE_unsigned) |
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -766,8 +766,19 @@ void DwarfUnit::constructTypeDIE(DIE &Buffer, const DIBasicType *BTy) { | |||||||||||||
| addUInt(Buffer, dwarf::DW_AT_encoding, dwarf::DW_FORM_data1, | ||||||||||||||
| BTy->getEncoding()); | ||||||||||||||
|
|
||||||||||||||
| uint64_t Size = BTy->getSizeInBits() >> 3; | ||||||||||||||
| addUInt(Buffer, dwarf::DW_AT_byte_size, std::nullopt, Size); | ||||||||||||||
| uint64_t SizeInBytes = divideCeil(BTy->getSizeInBits(), 8); | ||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know GCC emits both bit-size and byte-size, but is the byte-size here really useful if we have the bit-size?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm personally in favour of doing whatever is least surprising. I need to check with our debugger folks if they'd be ok with only keeping one, but if that's not the case we could stick it behind SCE tuning, if you feel strongly here about only emitting one? (@dwblaikie might also have opinions?) FWIW LLDB doesn't seem to understand bit_size in this context (see this comment), though I haven't tried the same example emitting only bit_size (it could reasonably be that seeing both is confusing it? equally it might just not expect bit_size in this context).
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume this thing has a byte storage size, even if the arithmetic is done exactly to the number of bits specified? So I'd imagine we could want both - though it'd be a bit out-of-DWARF-spec which says a base type should have only one or the other. I guess really the thing has a byte_size, and a "number of bits used for arithmetic" - it's not really the size of the object, it's to do with the encoding/how the bits are used. But I don't especially mind if we use the bit_size to encode "number of bits used for arithmetic", especially if GCC's already doing it. Maybe file a DWARF issue as well.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
My optimisitic interpretation of the DWARF spec is that it especially is it allows it for base types (chpt 5, page end of 103 to top of 104):
It says both "or" and "and" which is a bit ambiguous, but other types only say "or".
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh sorry, I misunderstood what you were saying about the byte storage size. I get it now, I'll look into it tomorrow
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I think the quoted parts you mentioned do refute/address my concerns - though perhaps the spec expects us to also emit a DW_AT_data_bit_offset of zero in this case? Again, I don't mind matching GCC if it's just a bit weird - though it wouldn't be hard or expensive to add DW_AT_data_bit_offset as well (especially if it's just always 0 and can be stored in the abbrev with implicit_const - but I guess that depends where the padding is and where the bits are stored (if they're LSB or MSB, etc))
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the missing data_bit_offset in this case is ok too - the bit I quoted continues on a few sentences later:
GCC gives us this DWARF: The only difference between that and my patched Clang is that our name is
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
At least for LLDB, I'd have to double check how the type-name ends up being displayed. If it just uses the llvm-project/clang/lib/AST/TypePrinter.cpp Lines 1483 to 1488 in 15d11eb
In fact including the bit-ness in the name might make this a bit harder for LLDB. But probably still doable. Not opposed to aligning with GCC either though. And might be useful for other consumers 🤷♂️ But maybe best for a separate patch? There was an LLDB issue around |
||||||||||||||
| addUInt(Buffer, dwarf::DW_AT_byte_size, std::nullopt, SizeInBytes); | ||||||||||||||
| if (BTy->getTag() == dwarf::Tag::DW_TAG_base_type) { | ||||||||||||||
| // DW_TAG_base_type: | ||||||||||||||
| // If the value of an object of the given type does not fully occupy the | ||||||||||||||
| // storage described by a byte size attribute, the base type entry may also | ||||||||||||||
| // have a DW_AT_bit_size [...] attribute. | ||||||||||||||
| // TODO: Do big endian targets need DW_AT_data_bit_offset? See discussion in | ||||||||||||||
| // pull request #164372. | ||||||||||||||
| if (uint64_t DataSizeInBits = BTy->getDataSizeInBits(); | ||||||||||||||
| DataSizeInBits && DataSizeInBits != SizeInBytes * 8) | ||||||||||||||
| addUInt(Buffer, dwarf::DW_AT_bit_size, std::nullopt, DataSizeInBits); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| if (BTy->isBigEndian()) | ||||||||||||||
| addUInt(Buffer, dwarf::DW_AT_endianity, std::nullopt, dwarf::DW_END_big); | ||||||||||||||
|
|
||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.