Page MenuHomePhabricator

Allow attributes with global variables

Authored by javed.absar on Apr 13 2017, 5:52 AM.



This patch extends llvm-ir to allow attributes to be set on global variables.

An RFC was sent out earlier by my colleague James Molloy:

A key part of that proposal was to extend LLVM-IR to carry attributes on global variables. This generic feature could be useful for multiple purposes. In our present context, it would be useful to carry user specified sections for bss/rodata/data.

Diff Detail


Event Timeline

javed.absar created this revision.Apr 13 2017, 5:52 AM

This is very useful, thanks for working on it!

I believe there are some passes that may not currently call GV::copyAttributesFrom when they probably should be (GlobalOpt has some cases IIRC), but that can be looked into when you actually implement the proposed section attributes.

208 ↗(On Diff #95110)

hasAttribute() functions would be useful here (I see you already have them in SingleSlotAttributeList).

jroelofs added inline comments.Apr 13 2017, 10:40 AM
567 ↗(On Diff #95110)

I think autobrief has been turned on, so no need for \brief on any of these.

jmolloy resigned from this revision.Apr 18 2017, 1:33 AM


It looks good to me, but it would as I wrote this particular patch originally :) For that reason I can't effectively review it, so I'll resign.


Thanks Jonathan/Tobias for the review.
I have removed 'brief' as suggested. Please have a look.
Best Regards, Javed.

jroelofs edited edge metadata.Apr 19 2017, 8:17 AM
jroelofs added a subscriber: jmolloy.

Sorry I didn't spot these earlier, but...

568 ↗(On Diff #95699)

Usage of this encourages a lot of: getAttributes().getAttributes() which seems odd. @jmolloy/@javed.absar did you consider making this an operator->()?

Either way, shouldn't this return by reference, and not value (avoiding the copy)?

204 ↗(On Diff #95699)

Should this be return by reference instead of value (avoiding the copy)?

javed.absar added inline comments.Apr 19 2017, 2:49 PM
568 ↗(On Diff #95699)

The AttributeList is thin (containing pointer to impl), so should not be a problem passing around by value. Other usages e.g. class Function handles AttributeList similarly by passing it around by value.

208 ↗(On Diff #95110)

Yes, sorry missed this. Will add them soon and send new patch

204 ↗(On Diff #95699)

Same point as above.

jroelofs added inline comments.Apr 19 2017, 3:03 PM
568 ↗(On Diff #95699)

Oh, ok. Cool.

I have now added the hasAttribute functions to GV as suggested. Thanks for catching this.
Best Regards

This revision is now accepted and ready to land.Apr 20 2017, 7:40 AM
rnk requested changes to this revision.Apr 20 2017, 11:03 AM

I don't think we need a new SingleSlotAttributeList type, we already have AttributeSet for that. Feel free to add more helpers to that to implement addAttribute etc.

550 ↗(On Diff #95934)

How is this not just an AttributeSet, now that we have that?

This revision now requires changes to proceed.Apr 20 2017, 11:03 AM
pcc added a subscriber: pcc.Apr 20 2017, 12:45 PM

Could this help us solve PR31759? I think not, because for !type metadata, attributes as they are currently implemented would not be sufficient, because they cannot be used to represent entities with "internal linkage", as is currently possible with a distinct MDNode.

So I am mildly opposed to this as it stands, and would prefer that we come up with a single representation for non-opaque global attachments that would be sufficient for !type and !absolute_symbol as well as for sections.

javed.absar updated this revision to Diff 97876.May 4 2017, 2:14 PM
javed.absar edited edge metadata.

Thanks Reid for the AttributeSet. I have added helpers to it and made changes to my implementation to use it. Please have a look.

Hi Reid:
Please could you have a look at the patch and let me know if its ok now.

ping! Reid/Jonathan?

jroelofs accepted this revision.May 10 2017, 9:44 AM

A few nits, otherwise LGTM.

965 ↗(On Diff #97876)

Too many spaces before this }

2834 ↗(On Diff #97876)

12 ) --> 12)

655 ↗(On Diff #97876)

Delete the extra newline.

321 ↗(On Diff #97876)

if ( GV --> if (GV

619 ↗(On Diff #97876)

The declaration for this one must have matching #if guards.

This revision was automatically updated to reflect the committed changes.
javed.absar marked 5 inline comments as done.