[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