llvmbot
commented
2 years ago @llvm/issue-subscribers-debuginfo
Open topolarity opened 2 years ago
llvmbot
commented
2 years ago @llvm/issue-subscribers-debuginfo
llvmbot
commented
2 years ago @llvm/issue-subscribers-lldb
DavidSpickett
commented
2 years ago I see on the linked zig issue something about it not reproducing with zig
What version of lldb are you using and did you keep the same version when testing the different versions of zig?
(I'm thinking about bisecting one or both hence the question)
topolarity
commented
2 years ago I'm afraid the comment on the Zig side is about reproduction on different platforms, not different LLVM versions. The issue reproduces on x86-64, but not AArch64.
I'm able to reproduce with both LLDB 14.0.6 and LLDB 15.0.1. A non-opaque-pointer version also reproduces with LLVM 14 for me:
define internal fastcc void @printTargetArch(i6 *%0) !dbg !2251 {
Entry:
%1 = load i6, i6 *null, align 1, !dbg !2258
store i6 %1, i6 *%0, align 1
ret void
}Unfortunately, I don't have LLVM <=13 on my machine to see if this ever worked
DavidSpickett
commented
2 years ago Ok well at least I can concentrate on llvm only, thanks for the isolated reproducer.
I'll see if I can reproduce then check if it worked prior.
DavidSpickett
commented
2 years ago This reproduces on AArch64 if I remove the data layout (not sure if that matters) and change the triple line. Happens all the way back to 12 so I think it's always been this way.
The assert is:
lldb: /home/david.spickett/llvm-project/clang/lib/AST/RecordLayoutBuilder.cpp:1494: void (anonymous namespace)::ItaniumRecordLayoutBuilder::LayoutWideBitField(uint64_t, uint64_t, bool, const clang::FieldDecl *): Assertion `!Type.isNull() && "Did not find a type!"' failed.This hack gets us past the problem:
diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp b/clang/lib/AST/RecordLayoutBuilder.cpp
index 6f3ede2ce42a..9a6aa43a56c9 100644
--- a/clang/lib/AST/RecordLayoutBuilder.cpp
+++ b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1482,12 +1482,10 @@ void ItaniumRecordLayoutBuilder::LayoutWideBitField(uint64_t FieldSize,
QualType Type;
for (const QualType &QT : IntegralPODTypes) {
+ Type = QT;
uint64_t Size = Context.getTypeSize(QT);
-
if (Size > FieldSize)
break;
-
- Type = QT;
}
assert(!Type.isNull() && "Did not find a type!");What is happening is we have a 2 bit wide field and we'd find char at 8 bits, break the for and never set the type. Whether this hack is correct I need to check the standards.
This is something to do with a "wide bitfield" which is one that is larger than the underlying type, just from skimming the comments. There is also this:
assert(Context.getLangOpts().CPlusPlus &&
"Can only have wide bit-fields in C++!");Which makes me wonder if zig should have got this far but we are calling this from lldb so it's probably the case that only within clang that's what you'd expect. Might be irrelevant if the code in LayoutWideBitField turns out not to be correct anyway.
I will find the set of rules for these wide bitfields.
DavidSpickett
commented
2 years ago The rule clang is applying is from https://itanium-cxx-abi.github.io/cxx-abi/abi.html#layout "Allocation of Members Other Than Virtual Bases".
In particular "If sizeof(T)8 < n, let T' be the largest integral POD type with sizeof(T')8 <= n.".
ItaniumRecordLayoutBuilder::LayoutBitField gets StorageUnitSize from the type's TypeInfo. To get that we end up down in ASTContext::getTypeInfoImpl. This sees that the type of the field is Type::Enum. Then it gets the integer type of that enum which returns void (meaning the underlying enum type is void). Void has a storage size of 0 bytes. Presumably void is a fallback because we don't have any 2 bit type to use.
That means that sizeof(T) is 0, 0*8 is 0 and therefore 0 < 2 is true. We go to look for a different type to represent it as but since 2 is < the 8 bits of char, we break before setting any type.
I suspect we've never hit this before because this has only been used on C++ code, or on code generated with types that are >= 8 bits. For example this change to the IR makes it work:
- !21 = !DIBasicType(name: "u2", size: 2, encoding: DW_ATE_unsigned)
+ !21 = !DIBasicType(name: "u2", size: 8, encoding: DW_ATE_unsigned)In that case the underlying type for the enum becomes char.
For the C++ case it's an error to assign a field bitsize greater than the underlying type's bitsize. So I'm not sure how one would hit this rule, but I didn't try particularly hard to find a way.
If you choose something like size 10 you also get char, which follows the rule but seems a bit odd. 16 is not <= 10 so it goes with the 8 bit type. But that's probably beside the point here.
So one might try to argue that zig shouldn't set a size < 8 but that means that any other arbitrarily sized type is suspect also, which doesn't seem right.
Seems like clang should handle the case where n < 8. Except that you can't make sizeof(T')*8 <= n when n < 8. So I don't see how we can stick to the rule there.
We could just have an escape hatch, if the underlying type is void assume char? Or maybe lldb could synthesize these types before we do all this work, if we know we're debugging zig.
Overall it's tricky because this path is intended for code coming from C++ where you can't trigger this afaict.
DavidSpickett
commented
2 years ago Going by https://ziglang.org/documentation/0.4.0/#Primitive-Types zig allows arbitrary sized integers:
In addition to the integer types above, arbitrary bit-width integers can be referenced by using an identifier of i or u followed by digits. For example, the identifier i7 refers to a signed 7-bit integer. The maximum allowed bit-width of an integer type is 65535.Well, or it's packing a 4 choice enum into 2 bytes automatically. Either way they're a valid concept in the language unlike C++ that has multiples of byte sizes.
So telling zig not to emit some of those seems like a bad path.
dwblaikie
commented
2 years ago It may be a valid concept in zig, but why is lldb trying to parse that DWARF using clang at all, then? I'd have thought/my understanding was that lldb only uses the clang parsing for C/C++/ObjC/ObjC++, no? What DW_AT_language is zig using?
topolarity
commented
2 years ago Thanks for investigating @DavidSpickett ! Some thoughts:
It looks like T' is only used for alignment for wide bitfield types (which afaik are legal C++, e.g. uint8_t field: 256 has 8 bits for its value and 248 bits for padding). It might be coherent to declare a theoretical void field: 16 would have alignment 1 (effectively no alignment), no bits for a value and 16 bits for padding.
On the other hand, I don't think void field: 2 is the right way to lower this Zig type, since the 2 bits would end up padding bits. Instead, we need the equivalent of _BitInt(2) field: 2 (new in C23).
AFAIK the ABI for _BitInt is not totally determined yet so this may still be murky. Experimenting with this in clang shows that _BitInt(2) reports a size corresponding to its byte-aligned sizeof, not its bit-precise size. Maybe that's what Zig should do too?
It may be a valid concept in zig, but why is lldb trying to parse that DWARF using clang at all, then? I'd have thought/my understanding was that lldb only uses the clang parsing for C/C++/ObjC/ObjC++, no? What DW_AT_language is zig using?
Zig uses DW_LANG_C99. DW_LANG_ZIG is coming in a future DWARF revision iirc
jryans
commented
2 years ago Zig uses
DW_LANG_C99.DW_LANG_ZIGis coming in a future DWARF revision iirc
It's true it isn't in a published version of the DWARF standard yet, but the language code has at least been allocated, so presumably it would be safe enough to start using.
dwblaikie
commented
2 years ago It may be a valid concept in zig, but why is lldb trying to parse that DWARF using clang at all, then? I'd have thought/my understanding was that lldb only uses the clang parsing for C/C++/ObjC/ObjC++, no? What DW_AT_language is zig using?
Zig uses
DW_LANG_C99.DW_LANG_ZIGis coming in a future DWARF revision iirc
That seems like the bug to me. If that weren't being done, lldb wouldn't be trying to parse this using a C parser with those expectations (admittedly, lldb still shouldn't crash - but that's difficult to achieve while providing the high fidelity C++ parsing - the C++ parser is robust to C++ source code, but not robust to the source of ASTs generated by even valid C++ DWARF, let alone trying to impose those rules on other languages).
I'd suggest using an extension language code, or the allocated code despite it not being published yet (that's the reason for the allocation system - so you don't have to wait for a published standard to use the code).
topolarity
commented
2 years ago Unfortunately that allocation is for the new DWARFv6 DW_AT_language_name (https://dwarfstd.org/ShowIssue.php?issue=210419.1). If we use the allocated Zig code (0x1c) for DW_AT_language in DWARFv5, it ends up referring to Rust.
FWIW, the crash in lldb reproduces even when changing the DW_LANG to a non-C language, like Rust.
dwblaikie
commented
2 years ago FWIW, the crash in lldb reproduces even when changing the DW_LANG to a non-C language, like Rust.
That surprises me - my understanding was that lldb only tried to use the Clang AST builder if the language is C++ or similar. Might be worth looking into what's happening there/how that happens?
But, yeah, I think all of these "Use Clang for XXX until there is a proper language plugin for it" are likely to produce a lot of problems because the Clang AST building code is unlikely to ever be robust enough to not crash, let alone to acceptably parse/understand these non-C-family languages.
DavidSpickett
commented
2 years ago which afaik are legal C++, e.g. uint8_t field: 256 has 8 bits for its value and 248 bits for padding
You're right, somehow I got an error when I tried this myself but checking again it is a warning.
Thanks all for explaining the DWARF side of things. I understand from that that Zig will never have a specific DW_AT_language language value. Which leaves us waiting for DWARFv6 for an official way to identify it.
We could make it manually selected, or find something else in the binary. How about the producer tag?
0x0000000b: DW_TAG_compile_unit
DW_AT_producer ("zig 0.10.0")
DW_AT_language (DW_LANG_C99)
DW_AT_name ("testme")It's probably stretching the intent of the field but given there's only one zig compiler that limits what could go wrong.
Or LLDB/clang could include the change I showed, on the grounds that C++ code will never have an underlying type that is < 8 bits so the impact is small. On the promise that it gets removed once proper support is possible.
Either way we end up teaching lldb about zig sooner or later.
dwblaikie
commented
2 years ago Zig can have a DW_AT_language value - there's a user extension range in the DWARF spec so you can add things even if they aren't sanctioned by the DWARF Committee/published specs. (though you could also ask the committee to give you an old DW_AT_language value to use officially before/until the new language encoding attributes get standardized).
I'm not an lldb decider, but I think it's a lost cause/likely a source of continued bugs to try to parse non-C-family DWARF with Clang - it's already a very brittle system, and this would add a whole lot more instability.
topolarity
commented
2 years ago Thanks all for explaining the DWARF side of things. I understand from that that Zig will never have a specific
DW_AT_languagelanguage value. Which leaves us waiting for DWARFv6 for an official way to identify it.
Did some digging, and actually Zig does have a provisional DW_AT_language allocation which was made before the accepted DW_AT_language_name: https://dwarfstd.org/ShowIssue.php?issue=190407.1 . Presumably we can built the ball rolling for language-specific support using 0x0026.
That said:
Or LLDB/clang could include the change I showed, on the grounds that C++ code will never have an underlying type that is < 8 bits so the impact is small. On the promise that it gets removed once proper support is possible.
I think that however LLDB/clang are planning to handle _BitInt is exactly how Zig will want to handle this. At the end of the day, Zig's arbitrary-width integers are LLVM's arbitrary-width integers and C23's _BitInt means that these will make it into C/C++ too (and are already usable as a language extension in clang).
The main thing to clarify IMO is whether it's correct to have LLDB handle bit-precise integer widths, or if in the debug data Zig should always report the padded sizeof the integer (in which case the bug is on the Zig side). Or should it be the bit-precise size rounded up to the nearest byte?
dwblaikie
commented
2 years ago Looks like Clang's had support for _BitInt for a while now ( https://reviews.llvm.org/D108643 ) - so probably reasonable to ask what DWARF and AST representation it uses for that, and emulate it.
(though, again, in general, I think it's ill advised to try to wedge other languages into lldb's already overburdened DWARF->Clang AST interface)
To reproduce:
Result:
Also reproduces with "llc-14 -opaque-pointers"
Downstream issue: https://github.com/ziglang/zig/issues/12929