This is an archive of the discontinued LLVM Phabricator instance.

Fix clang debug info irgen of i128 enums
ClosedPublic

Authored by rnk on Jul 22 2021, 12:46 PM.

Details

Summary

DIEnumerator stores an APInt as of April 2020, so now we don't need to
truncate the enumerator value to 64 bits. Fixes assertions during IRGen.

Split from D105320, thanks to Matheus Izvekov for the test case and
report.

Diff Detail

Event Timeline

rnk created this revision.Jul 22 2021, 12:46 PM
rnk requested review of this revision.Jul 22 2021, 12:46 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 22 2021, 12:46 PM

Small nit but otherwise LGTM :)

clang/lib/CodeGen/CGDebugInfo.cpp
3123
llvm/lib/IR/DIBuilder.cpp
255

Do I get it right, and this is not the first place that I noticed this, but the terminology here is a bit unconventional with regards to "Signed" vs "Negative"?

It looks like around the debug info code, an APSInt will be a signed positive value for representing a negative number, and an unsigned one to represent a positive value.

dblaikie added inline comments.Jul 22 2021, 1:27 PM
clang/lib/CodeGen/CGDebugInfo.cpp
3122

Is the value already signed appropriately?

Removing this line of code (and the bool IsSigned variable, so it doesn't break -Werror=unused-variable) doesn't cause any tests to fail, that I can see.

rnk added inline comments.Jul 22 2021, 2:31 PM
clang/lib/CodeGen/CGDebugInfo.cpp
3122

I'm afraid it might not be NFC. I took the cautious approach of trying to leave things exactly as they were. Enums in C can have surprisingly different behavior, IIRC.

llvm/lib/IR/DIBuilder.cpp
255

To my knowledge, we try not to store data in a sign-and-magnitude representation, it's always twos-complement. Meaning, APInt always stores a sequence of bits, typically interpreted as an unsigned positive integer. With outside information about the signedness of that data, you can answer the question of whether it is negative or not. APSInt adds the extra "signed" field.

rnk updated this revision to Diff 360978.Jul 22 2021, 2:31 PM
  • move
dblaikie added inline comments.Jul 22 2021, 2:56 PM
clang/lib/CodeGen/CGDebugInfo.cpp
3122

I'd rather not leave in haunted-graveyard-y code like that.

Could we assert that Value.getIsSigned == IsSigned? Then if there's a case where it isn't we'll have a test case/justification for the code?

rnk added inline comments.Jul 23 2021, 12:31 PM
clang/lib/CodeGen/CGDebugInfo.cpp
3122

I convinced myself that the signed-ness of the APSInt in the EnumConstantDecl always matches by looking at this code here:
https://github.com/llvm/llvm-project/blob/aee8457b8d4123d087c45aef95d14f24934fed53/clang/lib/Sema/SemaDecl.cpp#L18379

So far as I can tell, we always run that code, no matter whether LangOpts.CPlusPlus is set or not. That's enough for me.

rnk added inline comments.Jul 23 2021, 12:36 PM
clang/lib/CodeGen/CGDebugInfo.cpp
3122

Oh, I was wrong, your suggested change *does* break clang/test/CodeGen/enum2.c. My caution was warranted. :)

rnk updated this revision to Diff 361306.Jul 23 2021, 12:46 PM
  • add comments
mizvekov accepted this revision.Jul 23 2021, 12:56 PM
This revision is now accepted and ready to land.Jul 23 2021, 12:56 PM
dblaikie added inline comments.Jul 23 2021, 3:16 PM
clang/lib/CodeGen/CGDebugInfo.cpp
3122

Hrm, still not quite clear on what's going on. Seems like GCC actually does things differently - it does produce DWARF using the signedness of the literal to dictate the signedness of the enumerator, which seems inconsistent with the comment ("This matches the DWARF produced by GCC for C enums with positive enumerators" - well it's consistent with that statement (for positive enumerators) but inconsistent with what happens if you mix sign of enumerators - then GCC uses different encodings, but Clang doesn't (regardless of the representational difference - with isSigned true or false))

Looks like LLVM, for DWARF at least, uses the signedness of the enumeration's type ( https://github.com/llvm-mirror/llvm/blob/2c4ca6832fa6b306ee6a7010bfb80a3f2596f824/lib/CodeGen/AsmPrinter/DwarfUnit.cpp#L1406-L1427 ). (the BTF backend seems to respect the individual signedness)

So... mixed feelings? For DWARF it looks like the signed-ness representation doesn't matter/is ignored and only the signedness of the enumeration is used. GCC respects the signedness of different expressions - but I don't know that that's a good choice either, seems like it should represent the value of the enumerator proper, not of the expression used to initialize the enumerator.

And also - what's the deal with the APInt encoding? It looks like something in the IR printing correctly encodes the signedness of the APInt value:

$ clang-tot enum.c -g -c -emit-llvm -S -o - | grep Enumerator
!6 = !DIEnumerator(name: "A", value: 0)
!7 = !DIEnumerator(name: "B", value: -1)

(this is the output without the per-enumerator signedness initialization)
So how's that working & why do we carry the extra signedness in a boolean as well?

Sorry, feel free to ignore all this if it's not worth the rabbit-hole. I'm generally OK with this preserving the existing behavior - but it does raise a bunch of questions for me.

MaskRay accepted this revision.Jul 23 2021, 7:12 PM
MaskRay added inline comments.
clang/lib/CodeGen/CGDebugInfo.cpp
3122

Seems like GCC actually does things differently

Can you elaborate a bit with an example?

I played with this a bit and GCC does appear to behave similar to clang with this patch.

Because of disallowed C++11 narrowing, I cannot place an signed enumerator in a enumeration type with an unsigned underlying type:

GCC
error: enumerator value ‘-3’ is outside the range of underlying type ‘long unsigned int’
(clang can suppress this with -Wno-c++11-narrowing/-Wno-narrowing)

rnk added inline comments.Jul 23 2021, 7:28 PM
clang/lib/CodeGen/CGDebugInfo.cpp
3122

And also - what's the deal with the APInt encoding? It looks like something in the IR printing correctly encodes the signedness of the APInt value:

I believe LLVM IR integers are always interpreted as signed, but really it's just a bit pattern. It's an APInt. The signedness is stored as a separate boolean in DIEnumerator.

I played with this a bit and GCC does appear to behave similar to clang with this patch.

I think we were discussing behavior in C. Clang in C++ always implicitly casts the enumerator constant to the enum type, but in C it seems it doesn't. In C, the actual enumerators have a type of int, and the enum has it's own type. It doesn't make sense to me, but that's as far as I got before I said forget it, just do the NFC thing.

dblaikie accepted this revision.Jul 24 2021, 11:27 AM

Preserving existing behavior sounds OK - maybe some comment about that it might be good to remove so the next person who looks at it knows there's something not-quite-fully-reasoned here (& the comment about GCC's representation choice still seems off - GCC does use the signedness of the enumerators, not only the enumeration).

clang/lib/CodeGen/CGDebugInfo.cpp
3122

Re: @MaskRay - Here's the GCC per-enumerator difference I was observing/referring to:

$ gcc-tot enum.c -g && llvm-dwarfdump-tot a.out -debug-info -v | grep -A2 "_enumerator\|enumeration"
0x0000002d:   DW_TAG_enumeration_type [2] *
                DW_AT_name [DW_FORM_string]     ("X")
                DW_AT_encoding [DW_FORM_data1]  (DW_ATE_signed)
--
0x0000003d:     DW_TAG_enumerator [3]  
                  DW_AT_name [DW_FORM_string]   ("A")
                  DW_AT_const_value [DW_FORM_data1]     (0x00)
--
0x00000041:     DW_TAG_enumerator [4]  
                  DW_AT_name [DW_FORM_string]   ("B")
                  DW_AT_const_value [DW_FORM_sdata]     (-1)
blaikie@blaikie-linux2:~/dev/scratch$ g++-tot enum.c -g && llvm-dwarfdump-tot a.out -debug-info -v | grep -A2 "_enumerator\|enumeration"
0x0000002d:   DW_TAG_enumeration_type [2] *
                DW_AT_name [DW_FORM_string]     ("X")
                DW_AT_encoding [DW_FORM_data1]  (DW_ATE_signed)
--
0x0000003d:     DW_TAG_enumerator [3]  
                  DW_AT_name [DW_FORM_string]   ("A")
                  DW_AT_const_value [DW_FORM_data1]     (0x00)
--
0x00000041:     DW_TAG_enumerator [4]  
                  DW_AT_name [DW_FORM_string]   ("B")
                  DW_AT_const_value [DW_FORM_sdata]     (-1)
blaikie@blaikie-linux2:~/dev/scratch$ cat enum.c
enum X { A, B = -1};
int main() { enum X a; }

Re: @rnk yeah - I'm still confused, but preserving existing behavior seems sufficient. (&

llvm/lib/IR/DIBuilder.cpp
255

Yeah. -the bit that I don't understand is that the textual IR seems to represent the value with signedness regardless of the "IsUnsigned" field of the DIEnumerator. And the DWARF emission code doesn't use the signedness of the enumerator at all - looks like it's a remnant of earlier implementations, perhaps.

This revision was automatically updated to reflect the committed changes.

Preserving existing behavior sounds OK - maybe some comment about that it might be good to remove so the next person who looks at it knows there's something not-quite-fully-reasoned here (& the comment about GCC's representation choice still seems off - GCC does use the signedness of the enumerators, not only the enumeration).

@rnk - any thoughts on the comments? (the existing comment about GCC's emission seems incorrect to me (or subtly correct, but probably easy to misunderstand) - GCC does use different DWARF encodings for negative enumerators, depending on the enumerator) And seems like a comment about how this is maybe confusing/in need of some more touch-up might be handy for the next reader? (specifically that the current DWARF backend ignores the "isUnsigned" flag entirely, and relies on the signedness of the enumerator's underlying type instead - so maybe we could remove the isUnsigned flag?)

rnk added a comment.Jul 26 2021, 3:57 PM

Preserving existing behavior sounds OK - maybe some comment about that it might be good to remove so the next person who looks at it knows there's something not-quite-fully-reasoned here (& the comment about GCC's representation choice still seems off - GCC does use the signedness of the enumerators, not only the enumeration).

@rnk - any thoughts on the comments? (the existing comment about GCC's emission seems incorrect to me (or subtly correct, but probably easy to misunderstand) - GCC does use different DWARF encodings for negative enumerators, depending on the enumerator) And seems like a comment about how this is maybe confusing/in need of some more touch-up might be handy for the next reader? (specifically that the current DWARF backend ignores the "isUnsigned" flag entirely, and relies on the signedness of the enumerator's underlying type instead - so maybe we could remove the isUnsigned flag?)

I wasn't aware that the backend ignored this info. If that's the case, then I think we can carry the signedness over from the enumerator, update the enum2 IRgen test, and someone can update the DWARF backend later if they want to match GCC more closely. I think C and GCC's behavior is driven by a desire to make enumerators behave as close as possible to an integer literal macro definition, which could explain the per-enumerator signedness, similar to how certain ways of spelling a literal affect its type.

Preserving existing behavior sounds OK - maybe some comment about that it might be good to remove so the next person who looks at it knows there's something not-quite-fully-reasoned here (& the comment about GCC's representation choice still seems off - GCC does use the signedness of the enumerators, not only the enumeration).

@rnk - any thoughts on the comments? (the existing comment about GCC's emission seems incorrect to me (or subtly correct, but probably easy to misunderstand) - GCC does use different DWARF encodings for negative enumerators, depending on the enumerator) And seems like a comment about how this is maybe confusing/in need of some more touch-up might be handy for the next reader? (specifically that the current DWARF backend ignores the "isUnsigned" flag entirely, and relies on the signedness of the enumerator's underlying type instead - so maybe we could remove the isUnsigned flag?)

I wasn't aware that the backend ignored this info. If that's the case, then I think we can carry the signedness over from the enumerator, update the enum2 IRgen test, and someone can update the DWARF backend later if they want to match GCC more closely. I think C and GCC's behavior is driven by a desire to make enumerators behave as close as possible to an integer literal macro definition, which could explain the per-enumerator signedness, similar to how certain ways of spelling a literal affect its type.

Yeah, perhaps that's the motivation, though it seems like somewhat of a lost cause, I think? Since there are lots of spellings of an enumerator value where this property wouldn't be preserved (a call to a constexpr function, for instance) - so it seems to me that the value would be better off using the domain of the enumeration rather than something about how an enumerator was spelled.

Ah well, hopefully this discussion here will be findable if/when someone wants to look into it more in the future. Thanks!

This broke determinism when building Firefox.

This broke determinism when building Firefox.

I'm curious: can you share an example dwarfdump diff that shows what is non-deterministic?

rnk added a comment.Nov 5 2021, 1:21 PM

Reviewing the code, I don't see any obvious sources of non-determinism (hash iteration). The only source I can imagine is uninitialized memory usage in the APInt. Let me know if this can be repro'd somehow.

It seems to have been fixed in rG3c47c5ca13b8a502de3272e8105548715947b7a8.

Actually, there are some remaining cases not covered by that. I'm trying to produce a small reproducer.

rnk added a subscriber: hans.Nov 11 2021, 10:59 AM

It seems to have been fixed in rG3c47c5ca13b8a502de3272e8105548715947b7a8.

Are you sure you've arrived at the right code review? This change, D106585, modified type information, which is unrelated to the change you linked to. We have also noticed *other* determinism issues (follow up with @hans to learn more), but they aren't related to this change, which is from July.

It seems to have been fixed in rG3c47c5ca13b8a502de3272e8105548715947b7a8.

Are you sure you've arrived at the right code review? This change, D106585, modified type information, which is unrelated to the change you linked to. We have also noticed *other* determinism issues (follow up with @hans to learn more), but they aren't related to this change, which is from July.

Yes, cherry-picking rG3c47c5ca13b8a502de3272e8105548715947b7a8, rGbadcd585897253e94b7b2d4e6f9f430a2020d642 and reverting the changes to clang/lib/CodeGen/CGDebugInfo.cpp from rG323049329939becf690adbeeff9f5f7e219075ec and rGf9f56488e02d1c09a9cd4acde61ce1c712e71405 fixes the non-determinism in Firefox with clang 13.0.0.

glandium added a comment.EditedDec 3 2021, 3:58 AM

So far, what I've found is that in some cases, DIEnumerator::get returns the same node for similar enumerator values in different enums that happen to have the same name and value (even if their bitwidth differs), but sometimes not.
For example, in one compilation I see:

DIEnumerator::get (Name={Data@0x5575ee17ea38="nsNumberControlFrame_id", Length=23}, IsUnsigned: optimized out, Value={U={VAL=50, pVal=0x32}, BitWidth=8}, Context@0x5575e8305da0.pImpl=0x5575e8307230) = 0x55760563a708
DIEnumerator::get (Name={Data@0x5575ee17ea38="nsNumberControlFrame_id", Length=23}, IsUnsigned: optimized out, Value={U={VAL=50, pVal=0x32}, BitWidth=32}, Context@0x5575e8305da0.pImpl=0x5575e8307230) = 0x5576067e7148

In another compilation of the same file with the same flags:

DIEnumerator::get (Name={Data@0x561c659e0cc8="nsNumberControlFrame_id", Length=23}, IsUnsigned: optimized out, Value={U={VAL=50, pVal=0x32}, BitWidth=8}, Context@0x561c5fb68da0.pImpl=0x561c5fb6a230) = 0x561c7ce9dbf8
DIEnumerator::get (Name={Data@0x561c659e0cc8="nsNumberControlFrame_id", Length=23}, IsUnsigned: optimized out, Value={U={VAL=50, pVal=0x32}, BitWidth=32}, Context@0x561c5fb68da0.pImpl=0x561c5fb6a230) = 0x561c7ce9dbf8

(it's worth noting that both enums have, like, 155 items, all of which are identical (except for their bitwidth), and only one of them is deduplicated this way, sometimes)

glandium added a comment.EditedDec 3 2021, 4:32 AM

Reduced testcase:

enum FrameIID {
  nsBox_id,
  nsIFrame_id,
  nsHTMLFramesetBlankFrame_id,
  nsHTMLFramesetBorderFrame_id,
};

enum class ClassID : unsigned char {
  nsBox_id,
  nsIFrame_id,
  nsHTMLFramesetBlankFrame_id,
  nsHTMLFramesetBorderFrame_id,
};

extern void foo(FrameIID, ClassID);

void bar(FrameIID f, ClassID c) { foo(f, c); }

Compile with clang++ -o test.ll -S -emit-llvm -g test.cpp a number of times, and observe that sometimes the output is different (about 1 in 20 on my machine). Add items to the enums to make it more frequent.

rnk added a subscriber: aeubanks.Dec 3 2021, 8:17 AM

Thanks for the reduction, it sounds like there is something wrong with the way DIEnumerator is uniqued in the LLVMContext. I probably don't have time to follow up on this, but maybe @dblaikie and @aeubanks can help out.

rnk added a comment.Dec 3 2021, 8:22 AM

This usage of isSameValue seems suspicious: https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/LLVMContextImpl.h#L389

It seems to allow the possibility that APInts of differing bitwidth compare equal, but the hash value incorporates the bitwidth, so they may be inserted into differing hash buckets.

This usage of isSameValue seems suspicious: https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/LLVMContextImpl.h#L389

It seems to allow the possibility that APInts of differing bitwidth compare equal, but the hash value incorporates the bitwidth, so they may be inserted into differing hash buckets.

yup, also checking the bit width makes the non-determinism go away, I'll send out a patch

This usage of isSameValue seems suspicious: https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/LLVMContextImpl.h#L389

It seems to allow the possibility that APInts of differing bitwidth compare equal, but the hash value incorporates the bitwidth, so they may be inserted into differing hash buckets.

yup, also checking the bit width makes the non-determinism go away, I'll send out a patch

https://reviews.llvm.org/D115054