This is an archive of the discontinued LLVM Phabricator instance.

Add LLVMGetAttrKindIDInContext in the C API in order to facilitate migration away from LLVMAttribute
ClosedPublic

Authored by deadalnix on Apr 3 2016, 6:26 PM.

Details

Summary

LLVMAttribute has outlived its utility and is becoming a problem for C API users that what to use all the LLVM attributes. In order to help moving away from LLVMAttribute in a smooth manner, this diff introduce LLVMGetAttrKindIDInContext, which can be used instead of the enum values.

Diff Detail

Event Timeline

deadalnix updated this revision to Diff 52518.Apr 3 2016, 6:26 PM
deadalnix retitled this revision from to Add LLVMGetAttrKindIDInContext in the C API in order to facilitate migration away from LLVMAttribute.
deadalnix updated this object.
deadalnix added a subscriber: llvm-commits.
whitequark added inline comments.Apr 3 2016, 11:12 PM
include/llvm-c/Core.h
480

You've just insisted on using size_t in another patch.

deadalnix updated this revision to Diff 52523.Apr 4 2016, 12:12 AM

Use size_t

whitequark added inline comments.Apr 4 2016, 12:14 AM
include/llvm-c/Core.h
480

Header doesn't match source.

deadalnix updated this revision to Diff 52524.Apr 4 2016, 12:17 AM

Messed up adding size_t . Now size_t actually added.

whitequark added inline comments.Apr 4 2016, 12:21 AM
include/llvm-c/Core.h
481

Do we need to introduce any new non-InContext variants? The utility seems marginal.

lib/IR/Core.cpp
129

getAttrKindFromName

131

You are using a deprecated API (from AttributeImpl.h):

// FIXME: Remove this!
static uint64_t getAttrMask(Attribute::AttrKind Val);

It will stop working properly once we have more than 64 attributes, which is likely quite soon.

Wallbraker edited edge metadata.Apr 4 2016, 5:23 AM

Will this for "uwtable" return the same value as LLVMAttribute.UWtable?

@Wallbraker yes it returns the same value, that's the whole point.

deadalnix added inline comments.Apr 4 2016, 10:10 AM
lib/IR/Core.cpp
131

Yes that's the whole point. Removing the old enum would provide no migration path for users, which is a problem. By creating a function that give you the enum values, wee open the door for non stable enum values and will be able to change them later. This is going to be a long transition, but after talking with @Wallbraker , it seems like a much better (and longer) way forward.

So the battle plan is:

  • Land this. LLVMGetAttrKindIDInContext and enum value match.
  • Wait for this to be released.
  • In the release after this is released, get rid of the enum, and possibly change what LLVMGetAttrKindIDInContext returns.
  • Remove the deprecated API.
deadalnix updated this revision to Diff 52579.Apr 4 2016, 10:20 AM
deadalnix edited edge metadata.

getAttrKindFromName

deadalnix updated this revision to Diff 52620.Apr 4 2016, 1:34 PM

Remove non in context verison of the function

I want a second opinion on this issue.

So what this commit is doing is adding a new function, that people are supposed to use to get the value instead of using the enums in the header. At a later point the semantics will change for the old functions and they will only accept the values retrieved from the new function. Essentially breaking API, if we count changing the meaning of values the same as adding/removing arguments.

Now in the past when breaking the API (as specified above) we have added new functions keeping the old one around for a release or two and then removing the old ones (see LLVMLinkModules2). The advantage of removing the symbols is that people who use not-capable-of-parsing-C-headers-languages will get linking error instead of harder to track down errors, like getting the wrong attribute on emitted functions.

Thoughts and comments please?

whitequark accepted this revision.Apr 13 2016, 8:45 AM
whitequark edited edge metadata.

LGTM. I agree with @Wallbraker that the functions accepting AttrKindIDs will have to have their ABI versions bumped.

This revision is now accepted and ready to land.Apr 13 2016, 8:45 AM
rafael edited edge metadata.Apr 13 2016, 1:45 PM
rafael added a subscriber: rafael.

So, I don't think we have *any* guarantees on the stability of the
name of attributes. Is this really something you want the C api to
access. If so, at the very least document that there is *no*
stability.

Cheers,
Rafael

@rafael I don't understand. How are you proposing frontends using LLVM-C emit functions with attributes?

@rafael , @whitequark , attribute id are not stable in LLVM. This is why getting the id from the name seems like the right path forward here.

@rafael The C enum doesn't help much since frontends using libffi to bind to the C API have to duplicate the enum in the binding. Whereas, the function will work for at least removed attributes (it can return 0 and signal an error to the frontend).

This revision was automatically updated to reflect the committed changes.

Whelp that was a bit fast.

There needs to be documentation on that function. Is there a list of attributes somewhere? What does it return on failure? How long are the values returned valid? Per context, per thread, per process, per release?

The advantage of enum is that its self documenting.