This is an archive of the discontinued LLVM Phabricator instance.

Add DIBuilder functions to build RAUWable DIVariables and DIFunctions.
ClosedPublic

Authored by friss on Sep 12 2014, 1:56 AM.

Diff Detail

Event Timeline

friss updated this revision to Diff 13619.Sep 12 2014, 1:56 AM
friss retitled this revision from to Add DIBuilder functions to build RAUWable DIVariables and DIFunctions..
friss added reviewers: echristo, dblaikie, aprantl.
friss updated this object.
friss added a subscriber: Unknown Object (MLST).
dblaikie added inline comments.Sep 12 2014, 9:20 AM
lib/IR/DIBuilder.cpp
1032

Should this be hardcoded as a definition? Seems like the "createTempStaticVariableFwdDecl" would want a declaration, not a definition.

1185

Should this take "isDefinition"? The function name indicates that it's only a declaration.

1207

Wonder if there's a nicer way to do this - templated & pass in a lambda?

But it's nothing drastic either way.

friss added inline comments.Sep 12 2014, 9:37 AM
lib/IR/DIBuilder.cpp
1032

Doh! you're right. I had it and then reworked the patch and lost that change...

1207

I had a /simpler/ way, but I'm not sure you'll find it nicer (In fact I changed it because I thought you'd not like it). In the first incarnation of the patch, I just checked the isDefinition value and decided upon temporary or not based on that. I audited all users and if I didn't miss anything, none passes false for isDefinition today. Thus only the ones I will introduce would need to care about the temporary aspect.

And similarly for the variables (where I introduced a new argument for isDefinition that I lost in this version of the patch).

dblaikie added inline comments.Sep 12 2014, 10:00 AM
lib/IR/DIBuilder.cpp
1041

How's this work? What will we do at the finalize step?

1207

Yeah - the only function declarations we create currently are member functions for which we use "createMethod" - though that's a subtle distinction that's probably not ideal to rely on (one day we might decide that createMethod can go away & we can just create declarations - declarations that would never be replaced with definitions).

What about passing in a function pointer? (&MDNode::getTemporary or &MDNode::get) Avoids a boolean argument (which is always a bit unclear at a call site without a comment) except we need to do other work (AllSubprograms and AllGVs)...

I guess we could rely on the fact that we only emit declarations when there's something that refers to them (so they don't necessarily need to be in retention lists), but that's still pretty subtle.

friss added inline comments.Sep 15 2014, 1:58 AM
lib/IR/DIBuilder.cpp
1041

I realise I hadn't replied to this. In followup patches, in CGDebugInfo::finalize(), forward declarations which have seen definitions are RAUWed and dropped, the other ones are placed into the corresponding retention list. They need to be in retention lists, not because thy would be lost otherwise, but simply because these lists are emitted first in DwarfUnit. And the imported_declaration stuff expects that all the referenced DIEs are already emitted at the time they are processed.

1207

I agree regarding the general meaning of a boolean at the call site. The function pointer would be nice if we didn't do any special processing depending on the value, but I thing testing for a function pointer of a certain value would look odd.

And regarding not putting the declarations in the retention lists, I replied above.

What about your first suggestion of a lambda? It would be pretty explicit.

friss updated this revision to Diff 13709.Sep 15 2014, 8:03 AM

Addressed review comments:

  • add isDefinition parameter to createStaticVariable
  • use a std::function and lambdas to convey the intent of the caller into createFunctionHelper and createStaticVariableHelper
dblaikie added inline comments.Sep 15 2014, 11:55 AM
lib/IR/DIBuilder.cpp
1041

Cna we just fix the imported_declaration stuff so it doesn't expect all the referenced DIEs to be already emitted (it can use the usual "getOrCreate" routines that lazily construct stuff if it's not already been constructed).

Also you said: " forward declarations which have seen definitions are RAUWed and dropped, the other ones are placed into the corresponding retention list." - they'll still need to be RAUW'd with themselves to promote them from temporary to non-temporary. I imagine this should look much the same as the code we already have for handling temporary type forward declarations, no? I'd expect to see it pretty much exactly the same/symmetric - is that not going to be the case?

1173

Since this is a static function, you can make it a template and avoid the std::function dynamic overhead - but I suppose that's a binary size V execution time tradeoff... I don't feel too strongly about it either way, though I'd probably have gone with a template rather than std::function, personally.

1248

Not sure what the style guide says, but I usually just capture by "[&]" rather than listing the me explicitly (goes for here and above)

friss added inline comments.Sep 15 2014, 12:16 PM
lib/IR/DIBuilder.cpp
1041

I'll try to look into the imported DIE emission code to see if we can remove constraints.

It's not exactly symmetric, but it looks pretty much the same. For example, in CGDebugInfo::finalise(), the loop handling the type expects that every type in the ReplaceMap has a counterpart in the TypeCache (I'm not totally clear how this is always true). For functions and variables, the handling loop allows definitions not to exist, but it's basically the same.

1173

Can do. I find the std::function more self documenting, but it has a sometimes noticeable overhead. I didn't think it would show up on profiles here, but if you prefer I'll happily change that to a template parameter.

1248

I haven't tried it, but I think [&] wouldn't capture 'this', as it can't be captured by reference. And wouldn't capturing everything by value create a lot of useless copies (in other words, does the compiler only copy what is needed?).

dblaikie added inline comments.Sep 15 2014, 1:52 PM
lib/IR/DIBuilder.cpp
1173

Nah, that's fine.

1248

I believe [&] does the right thing for 'this'

& yes, even when using a default capture, only things that are ODR-used within the lambda are captured. Things you don't reference won't be unnecessarily captured.

friss updated this revision to Diff 13750.Sep 16 2014, 6:39 AM

Addressed review comments: Do not capture explicitely in lambdas

dblaikie accepted this revision.Sep 16 2014, 10:40 AM
dblaikie edited edge metadata.

Looks good

This revision is now accepted and ready to land.Sep 16 2014, 10:40 AM
echristo accepted this revision.Sep 16 2014, 11:28 AM
echristo edited edge metadata.
friss closed this revision.Sep 17 2014, 2:38 AM
friss updated this revision to Diff 13774.

Closed by commit rL217949 (authored by @friss).