This is an archive of the discontinued LLVM Phabricator instance.

Add optional DICompileUnit to DIBuilder and make outlined debug info use that CU
ClosedPublic

Authored by paquette on Jan 18 2018, 11:18 AM.

Details

Summary

Previously, I committed a change to the outliner which added some debug information to outlined functions. This wasn't entirely great because DIBuilder only sets its CU using createCompileUnit. As a result, the outliner had to make a second compile unit just for the outlined sequences.

This sparked an e-mail discussion on the topic, so I'm putting the patch up here just because it's a bit easier to read + discuss.

The only change to DIBuilder is an added optional parameter in the constructor which allows you to set the CU if you already know what you want. There are no functional changes to it otherwise. The rest of the changes are in the outliner. It works like this:

  1. Keep track of the MachineFunction that each Candidate appears in. Use that to get a CU for that Candidate if possible.
  2. To find a CU for an OutlinedFunction, grab the first CU belonging to some Candidate associated with that function.
  3. When you create a function, see if it has a potential CU. If it does, then generate debug info for the outlined function.

Diff Detail

Event Timeline

paquette created this revision.Jan 18 2018, 11:18 AM
aprantl added inline comments.Jan 18 2018, 11:31 AM
lib/CodeGen/MachineOutliner.cpp
116

How about getCompileUnitOrNull?

179

Nitpick: there is no need to use \brief in Doxygen comments. LLVM uses the autobrief option.

181

perhaps shorter:

for (auto &C : Candidates) 
  if (auto *CU = C->getCUIfExists())
    return CU;
 return nullptr;
1282

Why not just allocate the DIBuilder on the stack?
DIBuilder DB(M, true, CU)

1283

Can you also grab the file from the DISubprogram that you got the CU from?

1290

I'd also put the linkage name here.

1291

0 /* Line 0 is reserved for compiler-generated code */

1292

rest looks good!

dblaikie added inline comments.Jan 18 2018, 11:34 AM
lib/CodeGen/MachineOutliner.cpp
116

Maybe "OrNull" rather than "IfExists". Maybe check if one's more prevalent than the other across the LLVM codebase - but either's fine, really.

182–188

I'd probably write that as:

for (const auto &C  : Candidates)
  if (auto *CU = C->getCUIfExists())
    return CU;
return nullptr;
1288–1295

Any chance of cloning the DISubprogram from the original function, then modifying it? Or does it seem better to create a separate one like this? I guess this is fine (Adrian?)

aprantl added inline comments.Jan 18 2018, 11:42 AM
lib/CodeGen/MachineOutliner.cpp
1288–1295

Since it will be a distinct MDNode modifying the clone is technically legal, but I don't consider it good practice. I think this approach is cleaner and easier to understand.

paquette updated this revision to Diff 130497.Jan 18 2018, 2:51 PM
paquette marked 8 inline comments as done.

Updated diff to reflect style notes and address review comments.

aprantl added inline comments.Jan 18 2018, 2:56 PM
lib/CodeGen/MachineOutliner.cpp
1283

Looks like this one was accidentally marked as done?

paquette added inline comments.Jan 18 2018, 3:05 PM
lib/CodeGen/MachineOutliner.cpp
1283

It's CU->getFile() on here now but in the e-mail thread, it appears to still be

DIFile *Unit = DB->createFile(CU->getName(), CU->getDirectory())

Maybe that got mixed up somehow? Or is that still incorrect? Is it possible for a DISubprogram to have a different DIFile than its CU?

aprantl added inline comments.Jan 18 2018, 3:38 PM
lib/CodeGen/MachineOutliner.cpp
1283

Is it possible for a DISubprogram to have a different DIFile than its CU?

Yes. For example, two functions defined in different header files have different files even if they belong to the same CU.

My original suggestion was to use SP->getFile() here where SP is the DISubprogram attached to the function we got the CU from.

That said, if we are outlining code from two function from two different files, this file will be wrong and I'm not sure how to handle this. Should we create a new "virtual" file "<outlined-code>"? What do others think?

probinson added inline comments.Jan 18 2018, 3:44 PM
lib/CodeGen/MachineOutliner.cpp
1283

I think taking the DIFile from the same DISubprogram where we got the CU is fine. It's more consistent than anything else we could do.

paquette updated this revision to Diff 130513.Jan 18 2018, 3:59 PM

Updated diff so that the file comes from the subprogram instead of the compile unit.

aprantl accepted this revision.Jan 18 2018, 6:10 PM
This revision is now accepted and ready to land.Jan 18 2018, 6:10 PM
paquette closed this revision.Jan 19 2018, 1:15 PM

Since it seems like everyone's concerns have been cleared up, I'm going to go ahead and commit this.

Thanks to everyone for their input! If there are any other concerns let me know.