[PATCH] D106585: Fix clang debug info irgen of i128 enums

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a subscriber: aeubanks.
rnk added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[Mike Hommey via Phabricator via cfe-commits]

glandium added a comment.

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. Add items to the enums to 
make it more frequent.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[Mike Hommey via Phabricator via cfe-commits]

glandium added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[Mike Hommey via Phabricator via cfe-commits]

glandium added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a subscriber: hans.
rnk added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[Mike Hommey via Phabricator via cfe-commits]

glandium added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[Mike Hommey via Phabricator via cfe-commits]

glandium added a comment.

It seems to have been fixed in rG3c47c5ca13b8a502de3272e8105548715947b7a8 
.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[Mike Hommey via Phabricator via cfe-commits]

glandium added a comment.

This broke determinism when building Firefox.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

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!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

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?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[Reid Kleckner via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG323049329939: Fix clang debug info irgen of i128 enums 
(authored by rnk).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGenCXX/debug-info-enum-i128.cpp
  llvm/include/llvm/IR/DIBuilder.h
  llvm/lib/IR/DIBuilder.cpp


Index: llvm/lib/IR/DIBuilder.cpp
===
--- llvm/lib/IR/DIBuilder.cpp
+++ llvm/lib/IR/DIBuilder.cpp
@@ -250,6 +250,11 @@
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);
Index: llvm/include/llvm/IR/DIBuilder.h
===
--- llvm/include/llvm/IR/DIBuilder.h
+++ llvm/include/llvm/IR/DIBuilder.h
@@ -181,7 +181,9 @@
  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);
Index: clang/test/CodeGenCXX/debug-info-enum-i128.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/debug-info-enum-i128.cpp
@@ -0,0 +1,15 @@
+// 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)
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -3084,15 +3084,17 @@
 
   SmallString<256> Identifier = getTypeIdentifier(Ty, CGM, TheCU);
 
-  // Create elements for each enumerator.
+  // Create elements for each enumerator. Use the signed-ness of the enum type,
+  // not the signedness of the enumerator. This matches the DWARF produced by
+  // GCC for C enums with positive enumerators.
   SmallVector Enumerators;
   ED = ED->getDefinition();
   bool IsSigned = ED->getIntegerType()->isSignedIntegerType();
   for (const auto *Enum : ED->enumerators()) {
-const auto  = 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.


Index: llvm/lib/IR/DIBuilder.cpp
===
--- llvm/lib/IR/DIBuilder.cpp
+++ llvm/lib/IR/DIBuilder.cpp
@@ -250,6 +250,11 @@
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);
Index: llvm/include/llvm/IR/DIBuilder.h
===
--- llvm/include/llvm/IR/DIBuilder.h
+++ llvm/include/llvm/IR/DIBuilder.h
@@ -181,7 +181,9 @@
  DIFile *File);
 
 /// Create a single enumerator value.
-DIEnumerator *createEnumerator(StringRef Name, int64_t Val, bool IsUnsigned = false);
+DIEnumerator 

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[David Blaikie via Phabricator via cfe-commits]

dblaikie accepted this revision.
dblaikie added a comment.

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).




Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:3117
+llvm::APSInt Value = Enum->getInitVal();
+Value.setIsSigned(IsSigned);
+Enumerators.push_back(DBuilder.createEnumerator(Enum->getName(), Value));

rnk wrote:
> MaskRay wrote:
> > dblaikie wrote:
> > > rnk wrote:
> > > > rnk wrote:
> > > > > dblaikie wrote:
> > > > > > rnk wrote:
> > > > > > > dblaikie wrote:
> > > > > > > > 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.
> > > > > > > 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.
> > > > > > 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?
> > > > > 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.
> > > > Oh, I was wrong, your suggested change *does* break 
> > > > clang/test/CodeGen/enum2.c. My caution was warranted. :)
> > > 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.
> > > 
> > > 
> > > 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)
> > 
> > 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 

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:3117
+llvm::APSInt Value = Enum->getInitVal();
+Value.setIsSigned(IsSigned);
+Enumerators.push_back(DBuilder.createEnumerator(Enum->getName(), Value));

MaskRay wrote:
> dblaikie wrote:
> > rnk wrote:
> > > rnk wrote:
> > > > dblaikie wrote:
> > > > > rnk wrote:
> > > > > > dblaikie wrote:
> > > > > > > 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.
> > > > > > 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.
> > > > > 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?
> > > > 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.
> > > Oh, I was wrong, your suggested change *does* break 
> > > clang/test/CodeGen/enum2.c. My caution was warranted. :)
> > 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.
> > 
> > 
> > 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)
> 
> 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, 

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[Fangrui Song via Phabricator via cfe-commits]

MaskRay accepted this revision.
MaskRay added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:3117
+llvm::APSInt Value = Enum->getInitVal();
+Value.setIsSigned(IsSigned);
+Enumerators.push_back(DBuilder.createEnumerator(Enum->getName(), Value));

dblaikie wrote:
> rnk wrote:
> > rnk wrote:
> > > dblaikie wrote:
> > > > rnk wrote:
> > > > > dblaikie wrote:
> > > > > > 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.
> > > > > 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.
> > > > 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?
> > > 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.
> > Oh, I was wrong, your suggested change *does* break 
> > clang/test/CodeGen/enum2.c. My caution was warranted. :)
> 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.
> 
> 
> 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)



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:3117
+llvm::APSInt Value = Enum->getInitVal();
+Value.setIsSigned(IsSigned);
+Enumerators.push_back(DBuilder.createEnumerator(Enum->getName(), Value));

rnk wrote:
> rnk wrote:
> > dblaikie wrote:
> > > rnk wrote:
> > > > dblaikie wrote:
> > > > > 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.
> > > > 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.
> > > 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?
> > 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.
> Oh, I was wrong, your suggested change *does* break 
> clang/test/CodeGen/enum2.c. My caution was warranted. :)
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.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[Reid Kleckner via Phabricator via cfe-commits]

rnk updated this revision to Diff 361306.
rnk added a comment.

- add comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGenCXX/debug-info-enum-i128.cpp
  llvm/include/llvm/IR/DIBuilder.h
  llvm/lib/IR/DIBuilder.cpp


Index: llvm/lib/IR/DIBuilder.cpp
===
--- llvm/lib/IR/DIBuilder.cpp
+++ llvm/lib/IR/DIBuilder.cpp
@@ -250,6 +250,11 @@
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);
Index: llvm/include/llvm/IR/DIBuilder.h
===
--- llvm/include/llvm/IR/DIBuilder.h
+++ llvm/include/llvm/IR/DIBuilder.h
@@ -181,7 +181,9 @@
  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);
Index: clang/test/CodeGenCXX/debug-info-enum-i128.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/debug-info-enum-i128.cpp
@@ -0,0 +1,15 @@
+// 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)
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -3108,15 +3108,17 @@
 
   SmallString<256> Identifier = getTypeIdentifier(Ty, CGM, TheCU);
 
-  // Create elements for each enumerator.
+  // Create elements for each enumerator. Use the signed-ness of the enum type,
+  // not the signedness of the enumerator. This matches the DWARF produced by
+  // GCC for C enums with positive enumerators.
   SmallVector Enumerators;
   ED = ED->getDefinition();
   bool IsSigned = ED->getIntegerType()->isSignedIntegerType();
   for (const auto *Enum : ED->enumerators()) {
-const auto  = 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.


Index: llvm/lib/IR/DIBuilder.cpp
===
--- llvm/lib/IR/DIBuilder.cpp
+++ llvm/lib/IR/DIBuilder.cpp
@@ -250,6 +250,11 @@
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);
Index: llvm/include/llvm/IR/DIBuilder.h
===
--- llvm/include/llvm/IR/DIBuilder.h
+++ llvm/include/llvm/IR/DIBuilder.h
@@ -181,7 +181,9 @@
  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, 

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:3117
+llvm::APSInt Value = Enum->getInitVal();
+Value.setIsSigned(IsSigned);
+Enumerators.push_back(DBuilder.createEnumerator(Enum->getName(), Value));

rnk wrote:
> dblaikie wrote:
> > rnk wrote:
> > > dblaikie wrote:
> > > > 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.
> > > 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.
> > 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?
> 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.
Oh, I was wrong, your suggested change *does* break clang/test/CodeGen/enum2.c. 
My caution was warranted. :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:3117
+llvm::APSInt Value = Enum->getInitVal();
+Value.setIsSigned(IsSigned);
+Enumerators.push_back(DBuilder.createEnumerator(Enum->getName(), Value));

dblaikie wrote:
> rnk wrote:
> > dblaikie wrote:
> > > 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.
> > 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.
> 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?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:3117
+llvm::APSInt Value = Enum->getInitVal();
+Value.setIsSigned(IsSigned);
+Enumerators.push_back(DBuilder.createEnumerator(Enum->getName(), Value));

rnk wrote:
> dblaikie wrote:
> > 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.
> 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.
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[Reid Kleckner via Phabricator via cfe-commits]

rnk updated this revision to Diff 360978.
rnk added a comment.

- move


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGenCXX/debug-info-enum-i128.cpp
  llvm/include/llvm/IR/DIBuilder.h
  llvm/lib/IR/DIBuilder.cpp


Index: llvm/lib/IR/DIBuilder.cpp
===
--- llvm/lib/IR/DIBuilder.cpp
+++ llvm/lib/IR/DIBuilder.cpp
@@ -250,6 +250,11 @@
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);
Index: llvm/include/llvm/IR/DIBuilder.h
===
--- llvm/include/llvm/IR/DIBuilder.h
+++ llvm/include/llvm/IR/DIBuilder.h
@@ -181,7 +181,9 @@
  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);
Index: clang/test/CodeGenCXX/debug-info-enum-i128.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/debug-info-enum-i128.cpp
@@ -0,0 +1,15 @@
+// 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)
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -3113,10 +3113,10 @@
   ED = ED->getDefinition();
   bool IsSigned = ED->getIntegerType()->isSignedIntegerType();
   for (const auto *Enum : ED->enumerators()) {
-const auto  = 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.


Index: llvm/lib/IR/DIBuilder.cpp
===
--- llvm/lib/IR/DIBuilder.cpp
+++ llvm/lib/IR/DIBuilder.cpp
@@ -250,6 +250,11 @@
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);
Index: llvm/include/llvm/IR/DIBuilder.h
===
--- llvm/include/llvm/IR/DIBuilder.h
+++ llvm/include/llvm/IR/DIBuilder.h
@@ -181,7 +181,9 @@
  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);
Index: clang/test/CodeGenCXX/debug-info-enum-i128.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/debug-info-enum-i128.cpp

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:3117
+llvm::APSInt Value = Enum->getInitVal();
+Value.setIsSigned(IsSigned);
+Enumerators.push_back(DBuilder.createEnumerator(Enum->getName(), Value));

dblaikie wrote:
> 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.
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.



Comment at: llvm/lib/IR/DIBuilder.cpp:255
+  assert(!Name.empty() && "Unable to create enumerator without name");
+  return DIEnumerator::get(VMContext, APInt(Value), Value.isUnsigned(), Name);
+}

mizvekov wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:3117
+llvm::APSInt Value = Enum->getInitVal();
+Value.setIsSigned(IsSigned);
+Enumerators.push_back(DBuilder.createEnumerator(Enum->getName(), Value));

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

Small nit but otherwise LGTM :)




Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:3118
+Value.setIsSigned(IsSigned);
+Enumerators.push_back(DBuilder.createEnumerator(Enum->getName(), Value));
   }





Comment at: llvm/lib/IR/DIBuilder.cpp:255
+  assert(!Name.empty() && "Unable to create enumerator without name");
+  return DIEnumerator::get(VMContext, APInt(Value), Value.isUnsigned(), Name);
+}

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106585/new/

https://reviews.llvm.org/D106585

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106585: Fix clang debug info irgen of i128 enums

[Reid Kleckner via Phabricator via cfe-commits]

rnk created this revision.
rnk added reviewers: mizvekov, dblaikie, MaskRay.
Herald added subscribers: dexonsmith, hiraditya.
rnk requested review of this revision.
Herald added projects: clang, LLVM.
Herald added a subscriber: llvm-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106585

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGenCXX/debug-info-enum-i128.cpp
  llvm/include/llvm/IR/DIBuilder.h
  llvm/lib/IR/DIBuilder.cpp


Index: llvm/lib/IR/DIBuilder.cpp
===
--- llvm/lib/IR/DIBuilder.cpp
+++ llvm/lib/IR/DIBuilder.cpp
@@ -250,6 +250,11 @@
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);
Index: llvm/include/llvm/IR/DIBuilder.h
===
--- llvm/include/llvm/IR/DIBuilder.h
+++ llvm/include/llvm/IR/DIBuilder.h
@@ -181,7 +181,9 @@
  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);
Index: clang/test/CodeGenCXX/debug-info-enum-i128.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/debug-info-enum-i128.cpp
@@ -0,0 +1,15 @@
+// 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)
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -3113,10 +3113,9 @@
   ED = ED->getDefinition();
   bool IsSigned = ED->getIntegerType()->isSignedIntegerType();
   for (const auto *Enum : ED->enumerators()) {
-const auto  = Enum->getInitVal();
-auto Value = IsSigned ? InitVal.getSExtValue() : InitVal.getZExtValue();
-Enumerators.push_back(
-DBuilder.createEnumerator(Enum->getName(), Value, !IsSigned));
+llvm::APSInt Value = Enum->getInitVal();
+Value.setIsSigned(IsSigned);
+Enumerators.push_back(DBuilder.createEnumerator(Enum->getName(), Value));
   }
 
   // Return a CompositeType for the enum itself.


Index: llvm/lib/IR/DIBuilder.cpp
===
--- llvm/lib/IR/DIBuilder.cpp
+++ llvm/lib/IR/DIBuilder.cpp
@@ -250,6 +250,11 @@
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);
Index: llvm/include/llvm/IR/DIBuilder.h
===
--- llvm/include/llvm/IR/DIBuilder.h
+++ llvm/include/llvm/IR/DIBuilder.h
@@ -181,7 +181,9 @@
  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