This is an archive of the discontinued LLVM Phabricator instance.

Fully handle globals and functions in CGDebugInfo::getDeclarationOrDefinition()
ClosedPublic

Authored by friss on Nov 7 2014, 10:55 AM.

Details

Summary

Currently this function would return nothing for functions or globals that
haven't seen a definition yet. Make it return a forward declaration that will
get RAUWed with the definition if one is seen at a later point. The strategy
used to implement this is similar to what's done for types: the forward
declarations are stored in a vector and post processed upon finilization to
perform the required RAUWs.

For now the only user of getDeclarationOrDefinition() is EmitUsingDecl(), thus
this patch allows to emit correct imported declarations even in the absence of
an actual definition of the imported entity.

(Another user will be the debug info generation for argument default values
that I need to resurect).

This clang patch actually requires another trivial patch to LLVM before it can
go in (to handle forward declared globals). However, the test I'd like to commit
for the LLVM part contains the IR generated by this patch, thus I would have
found it a bit strange to commit it before this one is approved.

Diff Detail

Repository
rL LLVM

Event Timeline

friss updated this revision to Diff 15933.Nov 7 2014, 10:55 AM
friss retitled this revision from to Fully handle globals and functions in CGDebugInfo::getDeclarationOrDefinition().
friss added reviewers: dblaikie, echristo.
friss updated this object.
friss added a subscriber: Unknown Object (MLST).
dblaikie accepted this revision.Nov 17 2014, 11:37 AM
dblaikie edited edge metadata.

Looks pretty reasonable. Some optional and/or follow-up patch comments.

Have you considered if/how some of this could be unified with the type decl-or-def handling?

lib/CodeGen/CGDebugInfo.cpp
2310 ↗(On Diff #15933)

If you want to submit these refactorings separately, feel free (& do that sort of thing without precommit review - just pulling out a bit of functionality and giving it a name is usually goodness even before the second user exists (we tend to have long functions that are hard to read - pulling out parts of them and giving them names usually makes them a bit easier to follow) - and it makes the code reviews more palatable (I can easily glance at a single function refactor like this, and mostly just assume you did the obvious thing of copy/paste the code and give it a name))

2310 ↗(On Diff #15933)

Though the name 'collect' might be different from the existing similar functions we have for subprograms - could you check if we can have a more consistent naming one way or the other? (I think I named the subprogram ones 'apply' rather than 'collect')

Oh... I was confused, thought this was an LLVM patch (since it was sent to llvm-commits) so I was talking about LLVM codegen/asmprinter stuff...

Nevermind.

2406 ↗(On Diff #15933)

Might be worth splitting the body of the if into its own function (& for symmetry, though it's shorter, the body of the else separately as well) so this function just immediately defers to variable or function handlers.

lib/CodeGen/CGDebugInfo.h
418 ↗(On Diff #15933)

Typo of 'Fucntion'

This revision is now accepted and ready to land.Nov 17 2014, 11:37 AM
In D6173#5, @dblaikie wrote:

Looks pretty reasonable. Some optional and/or follow-up patch comments.

I'll commit that with your feedback addressed.

Have you considered if/how some of this could be unified with the type decl-or-def handling?

I kinda did, but there were some unclear points that made me chose the separate handling approach. Maybe you can shed some light on those:

  • The TypeCache is keyed on the TagType itself. Unifying the map would mean keying on the canonical decl. I /think/ this should be fine, but I couldn't convince myself that I'm not missing something.
  • In the finalize phase, the type replacement logic asserts that there always is a replacement type registered in the TypeCache, while for decls there might not be one. I'm not actually sure where the guarantee comes from on the type side.
lib/CodeGen/CGDebugInfo.cpp
2310 ↗(On Diff #15933)

C'mon I did this mailing list mistake again... Sorry for the confusion.

In D6173#8, @friss wrote:
In D6173#5, @dblaikie wrote:

Looks pretty reasonable. Some optional and/or follow-up patch comments.

I'll commit that with your feedback addressed.

Have you considered if/how some of this could be unified with the type decl-or-def handling?

I kinda did, but there were some unclear points that made me chose the separate handling approach. Maybe you can shed some light on those:

  • The TypeCache is keyed on the TagType itself. Unifying the map would mean keying on the canonical decl. I /think/ this should be fine, but I couldn't convince myself that I'm not missing something.

I think that should be fine - I forget how it all works, but I believe it's possible to go back & forth between the type and its canonical declaration... (ASTContext probably provides the help there)

  • In the finalize phase, the type replacement logic asserts that there always is a replacement type registered in the TypeCache, while for decls there might not be one. I'm not actually sure where the guarantee comes from on the type side.

Yeah, we'd have to understand where that guarantee comes from. I think it might be because we (might) be creating stub metadata nodes before we even create the declaration (because we might end up with a definition and circular references, etc) - so the thing we're replacing is actually a stub, not even a declaration. That's only my wildest stab in the dark.

lib/CodeGen/CGDebugInfo.cpp
2310 ↗(On Diff #15933)

No worries, I'm sure I've done it once or twice.

friss closed this revision.Nov 17 2014, 7:41 PM
friss updated this revision to Diff 16317.

Closed by commit rL222220 (authored by @friss).