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

Fri Jul 23 12:36:20 PDT 2021

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