[PATCH] D105320: [CodeView] Saturate values bigger than supported by APInt.

[Reid Kleckner via Phabricator via cfe-commits]

rnk added subscribers: dblaikie, MaskRay, LemonBoy.
rnk added a comment.

I decided it would be faster and more convenient to respond in the form of a code review for clang, so here it is: D106585 <https://reviews.llvm.org/D106585>

Assuming that goes through, go ahead and rebase onto that after it lands.



================
Comment at: clang/test/CodeGenCXX/debug-info-codeview-int128.cpp:1
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -debug-info-kind=limited -gcodeview -S -emit-llvm %s -o - | FileCheck %s --check-prefix=LL
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -debug-info-kind=limited -gcodeview -S %s -o - | FileCheck %s --check-prefix=ASM
----------------
FWIW this actually isn't codeview specific. The existing code crashes if you compile for DWARF too:
https://gcc.godbolt.org/z/3ox8c87Y4

To channel @dblaikie , this test should be split up: ensure that clang produces the right LLVM IR, then add a codeview specific LLVM IR test case. We prefer to test the smallest testable unit.


================
Comment at: llvm/lib/DebugInfo/CodeView/CodeViewRecordIO.cpp:200
+      int64_t Val =
+          Value.getNumWords() <= 1U ? Value.getSExtValue() : INT64_MIN;
+      return writeEncodedSignedInteger(Val);
----------------
You can use `Value.isSingleWord()` for a simpler condition.


================
Comment at: llvm/lib/DebugInfo/CodeView/CodeViewRecordIO.cpp:204
+    uint64_t Val =
+        Value.getNumWords() <= 1U ? Value.getZExtValue() : UINT64_MAX;
+    return writeEncodedUnsignedInteger(Val);
----------------
This can be `Value.getLimitedValue()`.


================
Comment at: llvm/lib/DebugInfo/CodeView/CodeViewRecordIO.cpp:283
                                                 const Twine &Comment) {
+  // FIXME: There are no test cases covering this function.
   if (Value >= std::numeric_limits<int8_t>::min()) {
----------------
I think this particular method is uncovered because of a separate bug. We always consider enumerators to be unsigned:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp#L2120

I'm not sure if that's a bug or if it is intended, but that's why this is relatively uncovered.


================
Comment at: llvm/lib/IR/DIBuilder.cpp:249
   assert(!Name.empty() && "Unable to create enumerator without name");
   return DIEnumerator::get(VMContext, APInt(64, Val, !IsUnsigned), IsUnsigned,
                            Name);
----------------
Since DIEnumerator actually stores an APInt, can you add an overload that accepts an APSInt and call that from clang instead? The implementation will have to decompose the sign bit and slice out the APInt portion, but that seems like a better way of handling this.

I noticed the APInt change to DIEnumerator is new as of D62475, so April 2020. + at maskray @LemonBoy 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105320