Page MenuHomePhabricator

Add support for DW_AT_export_symbols for anonymous structs
ClosedPublic

Authored by shafik on Oct 14 2019, 4:35 PM.

Details

Summary

We add support for DW_AT_export_symbols to detect anonymous struct on top of the heuristics implemented in D66175

This should allow us to differentiate anonymous structs and unnamed structs.

We also fix TestTypeList.py which was incorrectly detecting an unnamed struct as an anonymous struct.

Diff Detail

Event Timeline

shafik created this revision.Oct 14 2019, 4:35 PM
aprantl accepted this revision.Oct 15 2019, 9:11 AM
This revision is now accepted and ready to land.Oct 15 2019, 9:11 AM

Have many compilers supported DW_AT_export_symbols for a while now? If not, are there any serious issues introduced here that would change debugger behavior if this attribute is not emitted by a compiler? Or is this a new fix in clang that was recently introduced in order to fix an issue when debugging in lldb?

shafik added a comment.EditedOct 16 2019, 10:35 AM

Have many compilers supported DW_AT_export_symbols for a while now? If not, are there any serious issues introduced here that would change debugger behavior if this attribute is not emitted by a compiler? Or is this a new fix in clang that was recently introduced in order to fix an issue when debugging in lldb?

We don't except any regressions for code compiled with older compilers. We are fixing the case that unnamed classes are identified as anonymous. The anonymous classes cases should be caught in older revisions in ClangASTContext::AddFieldToRecordType which does a Rec->setAnonymousStructOrUnion(true) for those cases.

This was originally motivated by D66175 which only partially fixed the cases of classes being incorrectly labeled as anonymous.

labath added a subscriber: labath.Oct 16 2019, 11:15 AM

Have many compilers supported DW_AT_export_symbols for a while now? If not, are there any serious issues introduced here that would change debugger behavior if this attribute is not emitted by a compiler? Or is this a new fix in clang that was recently introduced in order to fix an issue when debugging in lldb?

We don't except any regressions for code compiled with older compilers. We are fixing the case that unnamed classes are identified as anonymous. The anonymous classes cases should be caught in older revisions in ClangASTContext::AddFieldToRecordType which does a Rec->setAnonymousStructOrUnion(true) for those cases.

Would it make sense to write a test in asm then? That way it would be obvious exactly what kind of debug info is being tested, and you could ensure both old&new compiler behavior is covered.

Have many compilers supported DW_AT_export_symbols for a while now? If not, are there any serious issues introduced here that would change debugger behavior if this attribute is not emitted by a compiler? Or is this a new fix in clang that was recently introduced in order to fix an issue when debugging in lldb?

We don't except any regressions for code compiled with older compilers. We are fixing the case that unnamed classes are identified as anonymous. The anonymous classes cases should be caught in older revisions in ClangASTContext::AddFieldToRecordType which does a Rec->setAnonymousStructOrUnion(true) for those cases.

Would it make sense to write a test in asm then? That way it would be obvious exactly what kind of debug info is being tested, and you could ensure both old&new compiler behavior is covered.

The TestTypeList.py covers the relevant cases of both anonymous class and an unnamed class. I ran check-lldb with the feature for emitting DW_AT_export_symbols disabled and did not see any regressions in this test or others except `

We also have the lldb-cmake-matrix bot which runs the test suite using older versions of clang which is there for exactly this purpose to catch regressions due to features not supported by older compiler versions. Which would catch any regressions here.

shafik updated this revision to Diff 225508.Oct 17 2019, 1:32 PM

Minor fixes.

We also have the lldb-cmake-matrix bot which runs the test suite using older versions of clang which is there for exactly this purpose to catch regressions due to features not supported by older compiler versions. Which would catch any regressions here.

I'm afraid I have to disagree with that. Like I said the last time <D66370> when this topic came up, I don't think that this "matrix" testing is the way to test dwarf features. The "matrix" bot is very useful to catch regressions in parsing old debug info (because there will always be some quirk that you forget to think of), but I don't think there should be any piece of code that is _only_ tested this way, particularly when it is very easy to do so (like it is here).

As you're probably aware, there's an "end-to-end testing" discussion going on on the mailing lists right now. One of the main questions being debated is whether it is ok to have a test which checks the assembly generated from some c(++) source code. And there's a lot of pushback saying that this is testing too much. However, even such a test is peanuts compared to clang-ast-from-dwarf-unamed-and-anon-structs.cpp. Even though it's one of the simplest lldb test we have, it runs the compiler front end, back end *and* assembler, and then reads the output hoping that the pipeline has generated the thing it wants to test. (And btw, it will fail on windows.) It doesn't even check that the compiler has generated the thing it wants to check, so if the compiler output later changes to not include DW_AT_export_symbols, it will start to test something completely different, leaving the code added here uncovered, unless someone remembers to add a new row to the matrix bot.

Not that this is very likely to happen in this particular case, but these kinds of changes happen all the time. Just last week, llvm started generating a very different style of location lists. Both lldb <rL374769> and dsymutil <D69005> had to be fixed to handle that. Both patches had tests, which were not based on running clang to generate the debug info (the dsymutil test had a checked in binary, which I am not a fan of, but that's a different story). Even so, it's very likely that this has regressed our coverage of the previous forms of location lists because we had no tests (except some basic ones I've added two or three months ago) which tested this code directly. And we only know about this because the initial implementation was wrong/incomplete, so we noticed the breakage.

So, overall, I still think we should have an even more targeted test here. Maybe it's not fair to ask you to add this kind of a test for the "old" style structs because that is past and not really "on you". However, I think we should be trying to add these kinds of tests for new features whenever is possible (another thing -- I imagine debugging failures from the matrix bot can be hard, though I've fortunately had not had to do that yet). I've been trying to do that for the past year, and while it takes a while to get used to, after some time I've gotten fairly efficient at generating/reducing "raw" dwarf. Besides being able to generate "stable" tests, this also enables one to generate tests for "incorrect" dwarf, which is one of the things which is impossible with the "clang -g" approach.

This comment was removed by shafik.

We also have the lldb-cmake-matrix bot which runs the test suite using older versions of clang which is there for exactly this purpose to catch regressions due to features not supported by older compiler versions. Which would catch any regressions here.

I'm afraid I have to disagree with that. Like I said the last time <D66370> when this topic came up, I don't think that this "matrix" testing is the way to test dwarf features. The "matrix" bot is very useful to catch regressions in parsing old debug info (because there will always be some quirk that you forget to think of), but I don't think there should be any piece of code that is _only_ tested this way, particularly when it is very easy to do so (like it is here).

As you're probably aware, there's an "end-to-end testing" discussion going on on the mailing lists right now. One of the main questions being debated is whether it is ok to have a test which checks the assembly generated from some c(++) source code. And there's a lot of pushback saying that this is testing too much. However, even such a test is peanuts compared to clang-ast-from-dwarf-unamed-and-anon-structs.cpp. Even though it's one of the simplest lldb test we have, it runs the compiler front end, back end *and* assembler, and then reads the output hoping that the pipeline has generated the thing it wants to test. (And btw, it will fail on windows.) It doesn't even check that the compiler has generated the thing it wants to check, so if the compiler output later changes to not include DW_AT_export_symbols, it will start to test something completely different, leaving the code added here uncovered, unless someone remembers to add a new row to the matrix bot.

Not that this is very likely to happen in this particular case, but these kinds of changes happen all the time. Just last week, llvm started generating a very different style of location lists. Both lldb <rL374769> and dsymutil <D69005> had to be fixed to handle that. Both patches had tests, which were not based on running clang to generate the debug info (the dsymutil test had a checked in binary, which I am not a fan of, but that's a different story). Even so, it's very likely that this has regressed our coverage of the previous forms of location lists because we had no tests (except some basic ones I've added two or three months ago) which tested this code directly. And we only know about this because the initial implementation was wrong/incomplete, so we noticed the breakage.

So, overall, I still think we should have an even more targeted test here. Maybe it's not fair to ask you to add this kind of a test for the "old" style structs because that is past and not really "on you". However, I think we should be trying to add these kinds of tests for new features whenever is possible (another thing -- I imagine debugging failures from the matrix bot can be hard, though I've fortunately had not had to do that yet). I've been trying to do that for the past year, and while it takes a while to get used to, after some time I've gotten fairly efficient at generating/reducing "raw" dwarf. Besides being able to generate "stable" tests, this also enables one to generate tests for "incorrect" dwarf, which is one of the things which is impossible with the "clang -g" approach.

When the I added the feature to the front end tests were added to verify that DW_AT_export_symbols is being generated for anonymous structs in D66605 and D66667 so if this regresses in the front-end we will catch it vis these tests. So as far I can tell we have tests at every point it can regress.

When the I added the feature to the front end tests were added to verify that DW_AT_export_symbols is being generated for anonymous structs in D66605 and D66667 so if this regresses in the front-end we will catch it vis these tests. So as far I can tell we have tests at every point it can regress.

But that's a test for clang. It will make sure it clang does stop emitting this attribute accidentally, but it will not help you if the removal is a conscious decision. At that point, the clang test will be deleted/modified too, but I doubt anyone will think of lldb and the fact that some part of lldb codebase becomes untested.

In fact, I think these patches illustrate very well the point I'm trying to make. D66667 does not check that the attribute ends up in the debug info. It only adds a test to ensure that clang emits some llvm IR. It could test the actual dwarf, but it doesn't, because llvm has a strong preference for single-component unit tests. lldb's test suite is an exception in the llvm world in that nearly every lldb test is an end-to-end test.

shafik updated this revision to Diff 226349.Thu, Oct 24, 3:54 PM

-Add test to clarify what we depending wrt to debug info

aprantl added a comment.EditedThu, Oct 24, 4:08 PM
This comment has been deleted.
test/Shell/SymbolFile/DWARF/anon_class_w_and_wo_export_symbols.ll
1 ↗(On Diff #226349)

de it from time to time. I think in this case LLVM IR could be superior to x86 assembler since it is much shorter and easier to understand — even if we have to upgrade it from time to time. However, since this is explicitly testing for a DWARF feature, should we check the dwarfdump output that the attribute we are looking for is actually there?

I think in this case LLVM IR could be superior to x86 assembler since it is much shorter and easier to understand — even if we have to upgrade it from time to time. However, since this is explicitly testing for a DWARF feature, should we check the dwarfdump output that the attribute we are looking for is actually there?

test/Shell/SymbolFile/DWARF/anon_class_w_and_wo_export_symbols.ll
1 ↗(On Diff #226349)

[I don't know what happened to my comment here].

shafik updated this revision to Diff 226366.Thu, Oct 24, 5:45 PM
  • Updated test to add dwarfdump test
labath accepted this revision.Fri, Oct 25, 8:39 AM

Thanks for bearing with me. :)

test/Shell/SymbolFile/DWARF/anon_class_w_and_wo_export_symbols.ll
21 ↗(On Diff #226366)

llvm-dwarfdump

shafik updated this revision to Diff 226450.Fri, Oct 25, 9:57 AM
shafik marked an inline comment as done.

Using llvm-dwarfdump instead of dwarfdump

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMon, Oct 28, 3:28 PM

This is broken on Windows. I moved the Buildbot to staging to resolve some of the issues with the move to the monorepo, so you can see the failures here:

http://lab.llvm.org:8014/builders/lldb-x64-windows-ninja/builds/0/steps/test/logs/stdio