hctim added inline comments.
================ Comment at: llvm/test/tools/llvm-symbolizer/data-location.s:7 + +# CHECK: x +# CHECK-NEXT: 14572 4 ---------------- jhenderson wrote: > hctim wrote: > > jhenderson wrote: > > > hctim wrote: > > > > jhenderson wrote: > > > > > It might be useful to add comments about what is special about each > > > > > of these cases (i.e. data/bss/function-static/string) or even name > > > > > the variables along those lines to directly imply their purpose. That > > > > > way, if the test is ever rewritten, it's easy to understand why each > > > > > example exists and what is important about it. That being said, is > > > > > there actually a difference between them for the testing purposes? > > > > > What do they actually exercise in the code changes? (I haven't looked > > > > > at the code changes to understand this). > > > > Renamed them (and made the test less in need of massaging if things > > > > change). They're individually useful because they're different types of > > > > global variables, located in different places. > > > > They're individually useful because they're different types of global > > > > variables, located in different places. > > > > > > Right, but are they located in differenet places within the DWARF? In > > > other words, are any different code paths actually exercised this way? > > > > > > Tangentially related: is one of the cases missing from the debug_aranges, > > > so that the fallback code is exercised? > > > Right, but are they located in differenet places within the DWARF? In > > > other words, are any different code paths actually exercised this way? > > > > No, `bss_global` and `data_global` are functionally here. The reason why I > > have them both here is because if you compile the source file to an object > > rather than a DSO, you end up with bad address lookups because bss_global > > and data_global are both at address 0x0 (just in different sections). I > > figured this would give some amount of posterity here about why it's a DSO, > > I know that I was pretty surprised there wasn't a great way to test it. > > > > Maybe a comment is a better way to go about it. Let me know what you think. > > > > > Tangentially related: is one of the cases missing from the debug_aranges, > > > so that the fallback code is exercised? > > > > All of them are missing from the debug_aranges :) > As things stand, if I came to this test, I'd consider dropping vataiables > that appear to be in the same place in the debug info, so a comment is > probably needed saying what coverage they provide. Done, and also made this patch no longer depend on D123534. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123538/new/ https://reviews.llvm.org/D123538 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
