Fix clang debug info irgen of i128 enums

[rnk]

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.

[mizvekov]

Small nit but otherwise LGTM :)

clang/lib/CodeGen/CGDebugInfo.cpp

3121

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]

clang/lib/CodeGen/CGDebugInfo.cpp

3120

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]

clang/lib/CodeGen/CGDebugInfo.cpp

3120

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]

• move

[dblaikie]

clang/lib/CodeGen/CGDebugInfo.cpp

3120

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]

clang/lib/CodeGen/CGDebugInfo.cpp

3120

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]

clang/lib/CodeGen/CGDebugInfo.cpp

3120

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

[rnk]

• add comments

[dblaikie]

clang/lib/CodeGen/CGDebugInfo.cpp

3120

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]

clang/lib/CodeGen/CGDebugInfo.cpp

3120

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]

clang/lib/CodeGen/CGDebugInfo.cpp

3120

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]

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

3120

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.

[dblaikie]

In D106585#2902588, @dblaikie wrote:

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]

In D106585#2905663, @dblaikie wrote:

In D106585#2902588, @dblaikie wrote:

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.

[dblaikie]

In D106585#2905916, @rnk wrote:

In D106585#2905663, @dblaikie wrote:

In D106585#2902588, @dblaikie wrote:

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!

[glandium]

This broke determinism when building Firefox.

[aprantl]

In D106585#3110131, @glandium wrote:

This broke determinism when building Firefox.

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

[rnk]

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.

[glandium]

It seems to have been fixed in rG3c47c5ca13b8a502de3272e8105548715947b7a8.

[glandium]

In D106585#3122726, @glandium wrote:

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]

In D106585#3122726, @glandium wrote:

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.

[glandium]

In D106585#3125280, @rnk wrote:

In D106585#3122726, @glandium wrote:

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]

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}, [e-mail]=0x5575e8307230) = 0x55760563a708
DIEnumerator::get (Name={Data@0x5575ee17ea38="nsNumberControlFrame_id", Length=23}, IsUnsigned: optimized out, Value={U={VAL=50, pVal=0x32}, BitWidth=32}, [e-mail]=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}, [e-mail]=0x561c5fb6a230) = 0x561c7ce9dbf8
DIEnumerator::get (Name={Data@0x561c659e0cc8="nsNumberControlFrame_id", Length=23}, IsUnsigned: optimized out, Value={U={VAL=50, pVal=0x32}, BitWidth=32}, [e-mail]=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]

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]

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]

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.

[aeubanks]

In D106585#3169898, @rnk wrote:

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

[aeubanks]

In D106585#3170019, @aeubanks wrote:

In D106585#3169898, @rnk wrote:

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

clang/lib/CodeGen/CGDebugInfo.cpp

llvm::DIType *CGDebugInfo::CreateTypeDefinition(const EnumType *Ty) {
 
   SmallString<256> Identifier = getTypeIdentifier(Ty, CGM, TheCU);
 
   // Create elements for each enumerator.
   SmallVector<llvm::Metadata *, 16> Enumerators;
   ED = ED->getDefinition();
   bool IsSigned = ED->getIntegerType()->isSignedIntegerType();
   for (const auto *Enum : ED->enumerators()) {
-    const auto &InitVal = Enum->getInitVal();
-    auto Value = IsSigned ? InitVal.getSExtValue() : InitVal.getZExtValue();
+    llvm::APSInt Value = Enum->getInitVal();
+    Value.setIsSigned(IsSigned);
     Enumerators.push_back(
-        DBuilder.createEnumerator(Enum->getName(), Value, !IsSigned));
+        DBuilder.createEnumerator(Enum->getName(), std::move(Value)));
   }
 
   // Return a CompositeType for the enum itself.
   llvm::DINodeArray EltArray = DBuilder.getOrCreateArray(Enumerators);
 
   llvm::DIFile *DefUnit = getOrCreateFile(ED->getLocation());
   unsigned Line = getLineNumber(ED->getLocation());
   llvm::DIScope *EnumContext = getDeclContextDescriptor(ED);
   llvm::DIType *ClassTy = getOrCreateType(ED->getIntegerType(), DefUnit);
   return DBuilder.createEnumerationType(EnumContext, ED->getName(), DefUnit,

clang/test/CodeGenCXX/debug-info-enum-i128.cpp

+// RUN: %clang_cc1 %s -triple x86_64-windows-msvc -gcodeview -debug-info-kind=limited -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple x86_64-linux-gnu -debug-info-kind=limited -emit-llvm -o - | FileCheck %s
+
+enum class uns : __uint128_t { unsval = __uint128_t(1) << 64 };
+uns t1() { return uns::unsval; }
+
+enum class sig : __int128 { sigval = -(__int128(1) << 64) };
+sig t2() { return sig::sigval; }
+
+
+// CHECK-LABEL: !DICompositeType(tag: DW_TAG_enumeration_type, name: "uns", {{.*}})
+// CHECK: !DIEnumerator(name: "unsval", value: 18446744073709551616, isUnsigned: true)
+
+// CHECK-LABEL: !DICompositeType(tag: DW_TAG_enumeration_type, name: "sig", {{.*}})
+// CHECK: !DIEnumerator(name: "sigval", value: -18446744073709551616)

llvm/include/llvm/IR/DIBuilder.h

public:
     /// using the \p Parent relationship.
     /// \param Parent     Macro file parent (could be nullptr).
     /// \param Line       Source line number where the macro file is included.
     /// \param File       File descriptor containing the name of the macro file.
     DIMacroFile *createTempMacroFile(DIMacroFile *Parent, unsigned Line,
                                      DIFile *File);
 
     /// Create a single enumerator value.
-    DIEnumerator *createEnumerator(StringRef Name, int64_t Val, bool IsUnsigned = false);
+    DIEnumerator *createEnumerator(StringRef Name, APSInt Value);
+    DIEnumerator *createEnumerator(StringRef Name, int64_t Val,
+                                   bool IsUnsigned = false);
 
     /// Create a DWARF unspecified type.
     DIBasicType *createUnspecifiedType(StringRef Name);
 
     /// Create C++11 nullptr type.
     DIBasicType *createNullPtrType();
 
     /// Create debugging information entry for a basic

llvm/lib/IR/DIBuilder.cpp

 
 DIEnumerator *DIBuilder::createEnumerator(StringRef Name, int64_t Val,
                                           bool IsUnsigned) {
   assert(!Name.empty() && "Unable to create enumerator without name");
   return DIEnumerator::get(VMContext, APInt(64, Val, !IsUnsigned), IsUnsigned,
                            Name);
 }
 
+DIEnumerator *DIBuilder::createEnumerator(StringRef Name, APSInt Value) {
+  assert(!Name.empty() && "Unable to create enumerator without name");
+  return DIEnumerator::get(VMContext, APInt(Value), Value.isUnsigned(), Name);
+}
+
 DIBasicType *DIBuilder::createUnspecifiedType(StringRef Name) {
   assert(!Name.empty() && "Unable to create type without name");
   return DIBasicType::get(VMContext, dwarf::DW_TAG_unspecified_type, Name);
 }
 
 DIBasicType *DIBuilder::createNullPtrType() {
   return createUnspecifiedType("decltype(nullptr)");
 }