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

In https://reviews.llvm.org/D38588#896294, @yuyichao wrote:

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


I don't have a particularly good answer here. If a test could be a maintenance burden due to a possibly unstable API then it may not be worthwhile if the change it is testing is on the safe side. I'm hoping for input from a code owner to see whether they think introducing a UnitTest is worth it.

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

I'm thinking of just assuming that the calls like createTargetMachine are successful as you will be giving them a well defined target that you know all the parts exist for. I would have thought all of the make_error cases could be taken out at low risk.

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

It is a valid name, it could imply that you are testing some kind of debug output, at least that was my reaction when I scanned over it. Something like .foo will be fine. I also recommend taking out the debug generator specific comments such as /*DWARFMustBeAtTheEnd*/ and "we'll use to emit the DIEs."

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

Several of the longer running buildbots build and run the tests with something like -fsanitize=address or -fsanitize=memory. They may be able to pick up the error in the Arm reset() without as much provocation. I haven't thought about this much though so it may not help. Just trying to think of a way of making a smaller example. Yes it will still require a reset.

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

I was thinking of someone wandering across the unit test without any context in a few years time, I would expect that they would have to svn/git annotate to find the review to work out what the crash was, and why quite a large unit test was needed to reproduce it.  Yes I mean that it couldn't be written any other way. From a small investigation of the tools typically used in writing tests, llvm-mc only supports one input file and hence only one streamer, clang will create a new streamer for each object, so it won't end up reusing the last mapping symbol.


https://reviews.llvm.org/D38588