[PATCH] D40413: [CodeExtractor] Add debug locations for new call and branch instrs.

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:1050
+      }
+    assert(BranchI->getDebugLoc() && "Could not find debug location in header");
+  }
----------------
aprantl wrote:
> fhahn wrote:
> > rriddle wrote:
> > > fhahn wrote:
> > > > fhahn wrote:
> > > > > aprantl wrote:
> > > > > > This assertion will fail if you haved a nodebug function inlined into a function with debuginfo.
> > > > > Ah yes, I will look into how the inliner deals with that case and handle it appropriately.
> > > > The basic block `header` is from a function with debug info (it comes from `oldFunction`). Isn't it save to assume that instructions in this basic block should have debug info? Do you mean when inlining a nodebug function in one with debug info, some basic blocks could be missing debug info which could trigger this assertion in the partial inliner? (Sorry my knowledge of the assumptions  about debug info are quite light).
> > > It's not safe to assume that the header block will have debug locations. So it should probably just find the first valid debug location, if there is one, and use that for the call instruction. It's only needed if there is a valid debug location in the abstracted blocks anyways.
> > > 
> > > For nodebug functions that are inlined, all of the instructions being inlined inherit the debug location of the call. If there are missing debug locations due to that, then the function was already missing debug info on some instructions.
> > Ah thanks for confirming. That makes things slightly more tricky. What is the suggested way to find the "first" valid debug location? BFS through the CFG? Could it happen that  `getSubprogram()` is not null, but there are no debug locations?
> I think so:
> 
> ```
> __attribute__(nodebug) void f() { /* actual code */ }
> __attribute__(nodebug) void g() { f(); }
> void h() { g() }
> ```
> 
> After inlining f into g into h, all the code of f is in h, but it won't have debug locations.
If the header doesn't have a valid debug location it's going to be a bit wonky in terms of step debugging. At that point it's really just satisfying the need to have a location on the call. 

As for getting the location, you could either iterate the set of blocks being extracted or try and search the CFG. Searching the CFG will require logic for making sure that a particular BB is in the extracted set and not already checked. The extracted set could contain loops, multiple exits, etc. IMO it doesn't really seem worth the work for little to no benefit in the debugging experience.

For the last question, yes! CodeExtractor is a generic extraction utility, it needs to work in all potential scenarios. So it's possible that there could be a small set of block(s) in a function in which the instructions do not have debug locations. A more reasonable scenario for the partial inliner could be if a call to a nodebug function is inlined and the call has no debug location.

For the reverse, we may need to remove the check for the subprogram. For example this partial inline scenario:
```
void f() { /* Code */ }

__attribute__((nodebug)) void g() { 
   if( /* something */ ) {
     f();
     /* more code */
   }

   /* Other code */
}

void h() { g(); }
```

After inlining f into g the instructions still maintain their debug locations. If we extract the part containing the now inlined f, we will end up with debug locations and no subprogram. That creates a problem if we want to then inline the stub g into h.


https://reviews.llvm.org/D40413