[PATCH] D38588: Clear LastMappingSymbols and LastEMS(Info) when resetting the ARM(AArch64)ELFStreamer

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I don't have any objections to using a unittest, although I think their use is uncommon; I could only find AArch64 as the other example. It will be worth finding out what the Code Owners think as this type of test is somewhat sensitive to changes in llvm API so they may think the cost/benefit trade-off isn't worth it. Personally I would be happy with the change without a heavyweight test.

My understanding is that you have taken the example from unittests/DebugInfo/DWARF/DwarfGenerator.cpp and DWARFDebugInfoTest and cut it down to make a reproducer. In essence creating multiple OutStreamers, calling reset() to provoke the crash. I think that this approach could work but I think that it needs to be stripped down a lot further and explained a lot better with comments. To be more specific:

- We know that we are creating an Arm target and not from the host triple (DWARFDebugInfoTest) so we could cut out a lot of the error handling.
- Given that Generator is only instantiated once and most of the member variables aren't used outside create(), does there really need to be a separate generator class?
- Using a name like ".zdebug_foo" is confusing especially when we are talking about mapping symbols.
- Is there a smaller more direct set of function calls that can reproduce the crash? Maybe we don't have to if the sanitizer builds will catch it.
- I think the test needs a large comment explaining what it is trying to test and why it has to be a unittest. It is also worth mentioning that reusing a Streamer that it is reset() is viable use case for a JIT.


https://reviews.llvm.org/D38588