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

Mon Jul 1 18:13:20 PDT 2019

nilanjana_basu accepted this revision.
nilanjana_basu marked 2 inline comments as done.
nilanjana_basu added inline comments.
This revision is now accepted and ready to land.

================
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
----------------
dblaikie wrote:
> rnk wrote:
> > 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.
> 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.
Have reverted redundant tests (files that do not have type records). Will submit in the next patch.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D63662