This is an archive of the discontinued LLVM Phabricator instance.

[CodeView] Add support for emitting S_UDT for typedefs
ClosedPublic

Authored by majnemer on Jun 8 2016, 12:40 PM.

Details

Summary

Emit a S_UDT record for typedefs. We still need to do something for
class types.

Diff Detail

Repository
rL LLVM

Event Timeline

majnemer updated this revision to Diff 60083.Jun 8 2016, 12:40 PM
majnemer retitled this revision from to [CodeView] Add support for emitting S_UDT for typedefs.
majnemer updated this object.
majnemer added reviewers: rnk, aaboud.
majnemer added a subscriber: llvm-commits.
rnk added inline comments.Jun 8 2016, 12:53 PM
lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
780 ↗(On Diff #60083)

Why do we need this condition? Don't we want to emit typdefs at global scope? I'd expect we can reference function local typedefs from outside the function, with something like return type deduction:

auto f() {
  typedef int myty;
  struct A {
    myty a;
  };
  return A();
}
decltype(f()) gv;

Maybe that's contrived, but the reliance on type emission order here feels fragile.

I was envisioning that we'd have a vector of plain DIType*'s, and we'd iterate them at end of TU and call getCompleteTypeIndex on them, which would trigger emission of all non-forward decl record types.

majnemer added inline comments.Jun 8 2016, 1:09 PM
lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
780 ↗(On Diff #60083)

MSVC emits typedefs nested within functions in the same symbol section as the one for the function, I was trying to make sure that this would happen.
Consider if f where extern inline: where should the S_UDT go? I imagine MSVC will put it with the ProcSym for f which will be in a different section than the one for gv.

That being said, I think we can just never use the COMDAT section for the S_UDT records and always just stick them in the default section.
WTDYT?

rnk added inline comments.Jun 8 2016, 1:15 PM
lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
780 ↗(On Diff #60083)

What if we staple UDTs into the relevant FunctionInfo? Currently FnDebugInfo is indexed by Function*, but it could easily be changed to DISubprogram since the Function points to that.

majnemer added inline comments.Jun 8 2016, 1:50 PM
lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
780 ↗(On Diff #60083)

Hmm, I'm not sure how this helps. Today, we add functions to FnDebugInfo at beginFunction and we process them at endModule.

When would we be doing the stapling? We will only see the types when we are processing entities like locals. What if the local we are processing references a type that was defined in a prior FnDebugInfo entry?

aaboud added inline comments.Jun 8 2016, 1:57 PM
lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
780 ↗(On Diff #60083)

Not sure how this is possible.
But assume it is, I am not sure how your code above solve the case!
You are preventing emitting S_UDT in the wrong function, but you are ending up losing S_UDT because we are not processing types before we emitting symbols, which seems as a wrong design.

majnemer added inline comments.Jun 8 2016, 2:03 PM
lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
780 ↗(On Diff #60083)

Here is how it is possible:

extern inline auto f() {
  struct X { struct Y {}; };
  return X{};
}
void g() {
  decltype(f())::Y y;
}

I never claimed that my code solved the case and I'm quite aware that we are potentially dropping S_UDTs where we shouldn't. The question is: how do we fix it.

aaboud added inline comments.Jun 8 2016, 2:20 PM
lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
772 ↗(On Diff #60083)

Move this code to separate function (we will need it soon in other places).
For example:
std::string getScopedName(StringRef Name, DIScope *Scope)

780 ↗(On Diff #60083)

We should lower types at beginModule/beginFunction/beginInstruction, and S_UDT should be part of FnDebugInfo as Reid suggested.
This means that we will need also to change the map to FnDebuginfo to be indexed by DISubprogram rather than llvm::Function.
Finally, this means that we will not need to emit debug info for functions that have no DISubprogram (any reason why we are doing that today?)

In addition, we will need one set of S_UDT for the module scope.

rnk edited edge metadata.Jun 8 2016, 6:04 PM

We spent some time talking about this today, and I think there's two ways to do this:

  1. The hard way: compute type indices for all types used in the function's symbol substream before endFunction. Assert if someone calls getTypeIndex during .debug$S emission.
  2. The easy way: Don't handle the corner cases where people reference function-internal typedefs from outside that function, and just handle typedefs at file/namespace scope and function scope. This is David's current approach.

I think we should go with approach 2, but we should also remember file/namespace scope typedefs before committing this. I think it just requires an 'else if' in the existing code and a vector on CodeViewDebug. Together that will handle 99% of all typedef usage.

majnemer updated this revision to Diff 60188.Jun 9 2016, 9:42 AM
majnemer marked an inline comment as done.
majnemer edited edge metadata.
  • Address review comments
rnk accepted this revision.Jun 15 2016, 9:30 AM
rnk edited edge metadata.

lgtm

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
810 ↗(On Diff #60188)

Can you add a FIXME or TODO about how we would handle the 'else' case? We would have to perform all type translation before beginning emission of .debug$S, and then make LocalUDTs a member of FunctionInfo.

test/DebugInfo/COFF/udts.ll
4 ↗(On Diff #60188)

Paste the C++ source and say something about how this is testing for local UDTs? I'm guessing it's:

void f() {
  typedef int FOO;
  FOO f;
}
This revision is now accepted and ready to land.Jun 15 2016, 9:30 AM
This revision was automatically updated to reflect the committed changes.