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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/DebugInfo/CodeView/TypeRecordMapping.cpp:269
   uint16_t Size;
-  if (IO.isWriting()) {
+  if (!IO.isReading()) {
     ArrayRef<VFTableSlotKind> Slots = Record.getSlots();
----------------
rnk wrote:
> nilanjana_basu wrote:
> > dblaikie wrote:
> > > What else can you be doing other than reading or writing? (what is the semantic difference in this (& similar) code change?)
> > There are 3 modes - reading (for deserializing), writing (for serializing), & streaming (which uses MCStreamer to emit directives to the assembly file). 
> I think this makes some sense, since the new mode is "streaming", which is notionally similar to writing. If we're "not reading" then we are producing some kind of output, either bytes or symbolic structured data.
Hmm, I couldn't really see (from as much as a glance as I managed) how different the streaming and writing codepaths are - given that DWARF doesn't have this distinction. Could you describe why CodeView does? (what's different about the streaming V writing that motivates the need for both - having conditional code around computing complex comments is fine, but I wouldn't expect many other differences)


================
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.
Yeah, I tend to be a bit over-fussy about testing - in this case my premise is usually "could you write a bug that causes one path to fail and not the other & that wouldn't be caught by another test" - I'd have expected the difference between writing and streaming (if there needs to be one - short of skipping computing complex comments) to be at a relatively low layer of the stack - so many choices about the ways types are described in CodeView might happen up in common layers that are shared between streaming and writing. So having lots of tests that retest those same codepaths would be sub-optimal to me.

Not just with regards to the runtime cost of the tests (though that's part of it - moreso than the wall time on our machines when fully parallelized - but the buildbots, sanitizers, etc, and the sort of "death by a thousand cuts" situation of testing takes longer when lots of these choices are made) but also the maintenance of them - if you change the output in some way and have to touch more tests because of this, get screen fulls of failures that are all about the same bug, etc.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63662