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

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/LL

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

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 bi

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

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 inse

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

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 LL

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

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, nsHTMLFramesetBorderFr

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

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

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

glandium added a comment. In D106585#3125280 , @rnk wrote: > In D106585#3122726 , @glandium > wrote: > >> It seems to have been fixed in rG3c47c5ca13b8a502de3272e8105548715947b7a8 >>

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

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 yo

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

glandium added a comment. In D106585#3122726 , @glandium wrote: > It seems to have been fixed in rG3c47c5ca13b8a502de3272e8105548715947b7a8 > . Actually, there are some remaining cases

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

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

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

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

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

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 SI

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

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

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

dblaikie added a comment. In D106585#2905916 , @rnk wrote: > In D106585#2905663 , @dblaikie > wrote: > >> In D106585#2902588 , @dblaikie >> wrote: >> >>> Preserving exis

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

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 t

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

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 h

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

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

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

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

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

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

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

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

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

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 wrot

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

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-

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

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 wrot

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

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 wrot

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

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

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

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-i12

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

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 v

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

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

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

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: ll

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

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