[PATCH] D88988: [llvm-symbolizer] Add inline stack traces for Windows.

[Amy Huang via Phabricator via llvm-commits]

akhuang added inline comments.


================
Comment at: lld/test/COFF/symbolizer-inline.s:3-5
+# RUN: lld-link -entry:main -nodefaultlib %t.obj -out:%t.exe -pdb:%t.pdb -debug
+# RUN: llvm-symbolizer --obj=%t.exe --use-native-pdb-reader --relative-address \
+# RUN:   0x1014 0x1018 0x101c 0x1023 0x1024 | FileCheck %s
----------------
dblaikie wrote:
> rnk wrote:
> > dblaikie wrote:
> > > After updated the dependencies (adding a dependency on llvm-symbolizer from lld's tests so this test could run 27e73816d6f9a7e627db73c445c4329db2ecfeaf ) that got  me looking at this/thinking: If this is the first use of llvm-symbolizer in lld, maybe it's out of place here?
> > > 
> > > Indeed this patch made no changes to lld, so it seems unsuitable that tests be added to lld - changes to llvm should be tested from within llvm. For ELF, at least, we're leaning towards writing hand-crafted assembly and the assembling that (with llvm-mc) and running llvm-symbolizer on the assembled file. If that model would work for COFF that'd be great - but otherwise it is acceptable to include source and repro steps in a file, and checkin a binary file for running llvm-symbolizer over.
> > IMO it is important for test readability that we start with assembly, not a checked in binary file. It allows us to come up with creative .cv_loc transitions from one instruction to the next, and validate that we get the right source location at each instruction boundary.
> > 
> > There is prior art for using llvm-mc to produce an object file in llvm and then using llvm-symbolizer on that object file, but it's impossible to do the same for COFF. llvm-symbolizer expects to operate on a PDB file. The only tool capable of making a PDB from an object right now is LLD.
> > 
> > While it's unfortunate that the test lives in the wrong repo, the great increase in testability makes it worth it to me. Debug info is historically undertested or only tested via interactive debugger integration tests. I think there is a huge amount of value to this level (medium size integration?) of testing.
> > IMO it is important for test readability that we start with assembly, not a checked in binary file.
> 
> I agree that it's important - but to me, not at the cost of leaving the code untested in LLVM. That code is tested in the repository where it's committed seems like a fairly core value of the LLVM project in my experience.
> 
> > It allows us to come up with creative .cv_loc transitions from one instruction to the next, and validate that we get the right source location at each instruction boundary.
> 
> Not sure I follow here - a checked in binary, built from source including creative .cv_loc transitions, etc, could have the same test coverage, right?
> 
> > There is prior art for using llvm-mc to produce an object file in llvm and then using llvm-symbolizer on that object file,
> 
> Yeah, I still have sort of mixed feelings about that - but generally accepted the tradeoff (of increasing the size of the code underneath the test - including all of llvm-mc, rather than only the dumping/symbolizing code itself) in test maintainability is worthwhile, given the fairly limited complexity of functionality in llvm-mc that's required for most of these tests. (mostly don't care about eh_frame, so the line table generation is about the worst part of that - the rest of the DWARF is generated using basic assembly directives so the probability of bugs in llvm-mc messing with the tests is relatively low)
> 
> >  but it's impossible to do the same for COFF. llvm-symbolizer expects to operate on a PDB file. The only tool capable of making a PDB from an object right now is LLD.
> 
> Yeah, I thought that might be the case hence the caveat that maybe a checked in binary would be needed.
> 
> > While it's unfortunate that the test lives in the wrong repo, the great increase in testability makes it worth it to me.
> 
> I'm not sure it's any less testable, though - generating and checking in a binary doesn't seem like a huge hurdle to writing tests here. It was done in the past & while moving to assembly based tests has some value add, I don't think the checked in binaries testing libDebugInfo have been a major burden on LLVM development/maintenance.
> 
> > Debug info is historically undertested
> 
> I don't think this test is adding significantly more/different testing than I think would be reasonably requested otherwise. An LLVM test with a checked in binary and an lld test using assembly and a debug info dump of some kind (if that isn't already covered - I'd have assumed it was already covered when the relevant functionality was added to lld in other/previous patches?). 
> 
> > or only tested via interactive debugger integration tests. I think there is a huge amount of value to this level (medium size integration?) of testing.
> 
> I'm not sure I see this particular test providing a lot more value over testing the components separately. And even then, I think it's important to have the isolated testing in addition to any end-to-end testing.
> 
> There's the debuginfo-tests directory for more end-to-end testing, if that's desired (not sure if/how well that fits/is usable for COFF/PDB testing, though - could be worth expanding to support if it's not already testable there). Could have a full end-to-end test there, source code down to PDBs and symbolized. (might even be able to be portable between DWARF and PDB?)
> I agree that it's important - but to me, not at the cost of leaving the code untested in LLVM. That code is tested in the repository where it's committed seems like a fairly core value of the LLVM project in my experience.

We could also add a binary for testing in the llvm-symbolizer tests, where the other tests are. I didn't consider the fact that the code is untested in LLVM.

I agree that the assembly test doesn't provide any extra coverage that a checked in binary can't; I think the main upside of including the assembly test in lld is so that the test is easier to understand/modify. But maybe it's not too difficult to just include the assembly file in the llvm test inputs directory and re-build the binary whenever the test needs to be changed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88988