[PATCH] D63662: Changing CodeView Debug info Type record output

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

I think this is almost ready, I went to approve it but I found some more final things to fix before committing.



================
Comment at: llvm/include/llvm/DebugInfo/CodeView/CodeViewRecordIO.h:38
+
 class CodeViewRecordIO {
   uint32_t getCurrentOffset() const {
----------------
Please add some comments describing the three modes of this class.


================
Comment at: llvm/lib/DebugInfo/CodeView/CodeViewRecordIO.cpp:39
+
+  /* Padding to align with 4byte boundaries */
+  if (isStreaming()) {
----------------
Please use C++ style single line comments for consistency.


================
Comment at: llvm/lib/DebugInfo/CodeView/CodeViewRecordIO.cpp:238-250
+  if (isStreaming()) {
+    for (auto V : Value) {
+      if (auto EC = mapStringZ(V))
+        return EC;
+    }
+    Streamer->EmitIntValue(0, 1);
+  } else if (isWriting()) {
----------------
Maybe this could be simplified to avoid duplicate logic like this:
  uint8_t FinalZero = 0;
  mapInteger(FinalZero);


================
Comment at: llvm/lib/DebugInfo/CodeView/TypeRecordMapping.cpp:48-52
+  if (IO.isStreaming()) {
+    error(IO.mapStringZ(Name));
+    if (HasUniqueName)
+      error(IO.mapStringZ(UniqueName));
+  } else if (IO.isWriting()) {
----------------
We have to be careful here, this could introduce a bug when long class names are involved. The long-type-name.ll test case exists to exercise this code. I believe it doesn't fail because we write the type once, then read it, then stream it, so it gets truncated the first time around.

Is there anything wrong with using the isWriting() code path here, or should we use the !isReading pattern? It seems the same.


================
Comment at: llvm/test/DebugInfo/COFF/asm.ll:2
 ; RUN: llc -mcpu=core2 -mtriple=i686-pc-win32 -O0 < %s | FileCheck --check-prefix=X86 %s
+; RUN: llc -mcpu=core2 -mtriple=i686-pc-win32 -o - -O0 -filetype=obj < %s | llvm-readobj -S --sr --codeview | FileCheck --check-prefix=OBJ32 %s
 ; RUN: llc -mcpu=core2 -mtriple=i686-pc-win32 -o - -O0 < %s | llvm-mc -triple=i686-pc-win32 -filetype=obj | llvm-readobj -S --sr --codeview | FileCheck --check-prefix=OBJ32 %s
----------------
rnk wrote:
> nilanjana_basu wrote:
> > dblaikie wrote:
> > > Seems like overkill to double all these tests, perhaps?
> > These tests emit & check the same end result, but they go through different paths of the code. One creates the code view record from the object file directly, & checks it. The other one creates the assembly file (the current changes get executed in this path), converts it to object file, & then checks the code view record from it. This is to double check that both the newly added code, & the original code path work correctly. Also, when I run all the tests in COFF using llvm-lit, there is a delay of 0.6s added (originally runtime 1.4s, now takes 2s).
> My recommendation was to test both codepaths, direct object emission and via textual assembly and confirm that info comes out the same. But, this test and many others don't exercise type information, so this extra run isn't really testing anything.
> 
> I would recommend reverting changes to test files that don't contain any lines matching `LF_`, since those are an indication of the presence of type records.
I think I would be fine with leaving the extra testing in then if it's short. Or you could remove it. Either way is fine with me.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63662