[PATCH] D89072: [CodeView] Emit static data members as S_CONSTANTs.

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: clang/test/CodeGenCXX/debug-info-static-member.cpp:144-147
+// For some reason, const_va is not emitted when the target is MS.
+// NOT-MS: !DIDerivedType(tag: DW_TAG_member, name: "const_va",
+// NOT-MS-SAME:           line: [[@LINE-3]]
+// NOT-MS-SAME:           extraData: i32 42
----------------
dblaikie wrote:
> Bug or feature? If it's a bug, probably should at least make this comment a "FIXME"
Feature. The easiest way to understand it is to hallucinate the C++17 `inline` keyword on MSVC static const integer data members with inline initializers. This metadata is probably emitted in MS mode, but it probably comes later on.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:3147
+    const DIScope *Scope = DTy->getScope();
+    std::string QualifiedName = getFullyQualifiedName(Scope, DTy->getName());
+
----------------
Let's move this past the continue so we don't construct the name unless the value is non-null.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:3169
+    CodeViewRecordIO IO(Writer);
+    if (isStaticConstUnsigned(DTy->getBaseType())) {
+      uint64_t Val = Value.getZExtValue();
----------------
This makes me suspect that we still have bugs in the original S_CONSTANT code. Looks like this is a pre-existing bug:
```
$ cat t.cpp
static const signed int gv_signed = 0xFFFFFFFF;
const signed int useit2() { return gv_signed; }

$ clang-cl -c -Z7 t.cpp  && llvm-pdbutil dump -symbols t.obj | grep -A1 S_CONST
       0 | S_CONSTANT [size = 28] `gv_signed`
           type = 0x1000 (const int), value = 18446744073709551615

$ cl -c -Z7 t.cpp  && llvm-pdbutil dump -symbols t.obj | grep -A1 S_CONST
Microsoft (R) C/C++ Optimizing Compiler Version 19.27.29111 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

t.cpp
       0 | S_CONSTANT [size = 21] `gv_signed`
           type = 0x1000 (const int), value = -1
```

Can you re-structure this so that we aren't duplicating the calls to mapEncodedInteger in two places? mapEncodedInteger takes an APInt, which carries a sign already. We just have to use the type information to correct the sign on Value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89072