[Lldb-commits] [PATCH] D105564: Fix for DWARF parsing to better handle auto return type for member functions

[Raphael Isemann via Phabricator via lldb-commits]

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

I'm really sorry, I don't understand what kind of review is expected here given the state of this patch.

- The patch **still** doesn't include the tests that I asked for in my previous comment: https://reviews.llvm.org/D105564#2871703
- I applied the patch locally and **again** LLDB just crashes on the first corner case that I test.
- I genuinely can't comprehend why my crash reproducer got copied one-to-one into this patch as a real test case. It's just meant to make it straightforward to reproduce the crash I ran into, but the 'test' itself does completely bogus things like checking for the *wrong* return value, declaring dead code, all the identifiers are just 'bla', etc...

I think it's part of a good review to actually compile and poke around at the code that I'm requested to review. It's also fine if I run into some weird corner cases while doing some manual testing (one could argue that's the whole point of the review. I'm always happy when someone invests the time to do that for my patches). But this patch feels like every reviewer either has to just hope this just works or IKEA-style assemble their own tests at home. This is fine for an RFC/WIP patch, but this is clearly aiming to be accepted at the moment.

Given how long everyone's review queue is, it seems frankly unfair to other people to postpone reviewing their patches and instead spend time repeatedly reviewing this patch by manually compiling and testing it. I'll send this patch back and I'm happy to review it once it's actually ready, but at the moment it's sadly not.

Some notes on what I expect to happen to this patch before this goes back in anyone's review queue:

- Get enough (documented) tests that a reasonable person can look at this patch and believe it could work in production.
- Obviously fix any bugs uncovered by the tests.
- I don't think the current approach will always correctly resolve the return type (which is perfectly fine), but if that happens we shouldn't crash. IIRC the reason why my original patch set the C++14 flag in the expr evaluator is because it makes using these raw 'auto' types an error diagnostic instead of a crash. In any case, this should be tested then. Just to be clear: I think getting this to work in every case is out of scope for this patch so I think it's fine if those cases are at just documented/tested.



================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:402-403
     case DW_AT_type:
-      type = form_value;
+      if (!type.IsValid())
+        type = form_value;
       break;
----------------
shafik wrote:
> JDevlieghere wrote:
> > What's the purpose of this? Do we expect to see the type attribute more than once? 
> Good question, the first iteration was done by @teemperor and then I modified it heavily but I did not dig  into this change to deeply but I was pretty sure when we first talked about there was a good reason for it.
`ParsedDWARFTypeAttributes` iterates not just over the attributes in the current DIE, but also follows `DW_AT_specification` and then iterates the attributes there. But the `DW_AT_specification` of the definition points to the declaration with the `auto` type, so without this change we would first assign `type` to the real type from the definition and then when we iterate over the attributes of the declaration we would overwrite the real type with the `auto` type.

In short: without this change `ParsedDWARFTypeAttributes(definition_die).type` would be `auto` instead of the real type.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105564/new/

https://reviews.llvm.org/D105564