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

[Yichao Yu via Phabricator via llvm-commits]

yuyichao added a comment.

> Personally I would be happy with the change without a heavyweight test.

I'm mainly not sure what I'm expected to do. E.g. I wasn't sure if the previous comment is about we don't need a test or a guide for how to add such a test. I added it mainly because removing the tests later is significantly easier.

> 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.

Only one `OutStreamer` but yes, that's roughly what I did.

> 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.

OK. I'm not really sure which ones are needed.

> 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?

Some of the member variables are keeping object lifetime so they needs to be there. Some of the pointer ones can probably be removed.
The separate class is mainly to make it clear what I'm actually testing and since there are multiple `unique_ptr`s and other objects it's easier to return them in a class. I can certainly move then all into the constructor if non of the error handling is needed though.

> Using a name like ".zdebug_foo" is confusing especially when we are talking about mapping symbols.

Is this actually a special name? I just figure it was used in another test so it must be valid. What would be better names? Just `.foo` and `.bar`?

> Is there a smaller more direct set of function calls that can reproduce the crash?

The actual function call is pretty direct I think? Just emitting something random while switching the section back and forth. The longest part is the generator, which is the only way I can figure out to create a `OutStreamer`. If there's more direct way to do that I can certainly change that part.

> Maybe we don't have to if the sanitizer builds will catch it.

Not sure if I understand this (and I've never used a sanitizer build before.....). Shouldn't it still require a reset on the out streamer to catch anything?

> 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.

I mentioned that I'm switching sections and test for no-crash. By "why it has to be a unittest" do you mean that no other llvm tools reset anything?


https://reviews.llvm.org/D38588