[PATCH] D45593: [DebugInfo][OPT] Fixing a couple of DI duplication bugs of CloneModule

[Roman Tereshin via Phabricator via llvm-commits]

rtereshin added inline comments.


================
Comment at: llvm/trunk/lib/IR/DebugInfo.cpp:141
+    else
+      llvm_unreachable("unexpected imported entity type");
+  }
----------------
aprantl wrote:
> rtereshin wrote:
> > aprantl wrote:
> > > aprantl wrote:
> > > > aprantl wrote:
> > > > > rtereshin wrote:
> > > > > > alberto_magni wrote:
> > > > > > > This commit is causing  failures in my SPEC2006 tests (xalancbmk among others) with -flto=thin and -g.
> > > > > > > The unreachable in this line gets triggered. 
> > > > > > > 
> > > > > > > This is because the if-cascade above does not handle the case for DIGlobalVariable.
> > > > > > > Probably the code that handles GlobalVariables at the top of the function needs to be replicated in one if-case here too.
> > > > > > Hi @alberto_magni,
> > > > > > 
> > > > > > Is there a chance you could extract a regression test from one of those SPEC2006 tests?
> > > > > > 
> > > > > > Thanks!
> > > > > > Roman
> > > > > If it's just a DIGlobalVariable appearing in that list, you should be able to construct one manually without much trouble.
> > > > ```
> > > > $ cat /tmp/using.cpp
> > > > namespace s {
> > > >   int i;
> > > > }
> > > > 
> > > > using s::i;
> > > > 
> > > > int f() { return i; }
> > > > ```
> > > > 
> > > > Using declarations are represented as imported entities.
> > > That also means that we'll need to accept pretty much anything in that list :-(
> > Thanks, I'll try that!
> > 
> > So, we gotta remove the unreachable, I suppose?
> Yes, but perhaps think about what else might appear in a using statement that we would need to process here and also whether it can be dangerous to process something twice, because it is reachable via the imported entities and some other means (as in my example above) so we don't end up with duplicates / infinite loops / etc.
I see.

I don't know off the top of my head how to turn this "global variable as an imported entity" example into a test that would undeniably break one of the `DebugInfoFinder` users given that `llvm_unreachable` is removed. `DebugInfoFinder` tracks types, and `CloneFunction` self-maps them for some reason, while `DIGlobalVariable` references a type, so maybe it may cause some debug info duplication if `CloneModule` is called?

There are also `ModuleDebugInfoPrinter` and `StripDeadDebugInfo` module passes, not sure what exactly they expect from `DebugInfoFinder`.

That being said, if approached meticulously, this is going to take some time.

Do you think we need to remove `llvm_unreachable` first right now before we do anything else?


Repository:
  rL LLVM

https://reviews.llvm.org/D45593