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 13 2016, 4:09 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.

See D18749 for reference.

Diff Detail

Event Timeline

deadalnix updated this revision to Diff 53636.Apr 13 2016, 4:09 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.

So as per request, let's explicit what the problem is here and what is the plan to move forward. The problem is that the attribute mapping in the C API does not allow to qualify everything (for instance, return values) and does not allow mapping of newer attributes. Because new attributes are added on a regular basis, the C API become more and more obsolete with each release.

The problem is know for a long time, but there is no obvious and straightforward path forward (I made patch about this more than 2 years ago).

To move forward, I propose to

  • Add a function that allow to get an attribute ID from the attribute name. As a first iteration, this function returns the same value as in the enum.
  • Wait for it to be released.
  • Remove the enum.

Then we can move forward.

Note that name aren't more stable than ids in theory. in practice they are much more stable and it is possible for the function to return an error code on missing attribute name as suggested. by @whitequark .

Wallbraker requested changes to this revision.EditedApr 13 2016, 11:22 PM
Wallbraker edited edge metadata.

About the code:
The function requires documentation. 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?

About the plan:
Since @whitequark also wanted to bump the ABI version of the KindID you might as well add of the new functions in one commit (new functions and keeping the old, yes its duplicated API but its only for one release).

Will target dependent attributes be covered by the API (like "no-sse")? Or is it only the regular attributes?

The advantage of enums is that they are self documenting, the disadvantage is that ffi bindings can get stale enums. (if we have rules that we can't reuse old values) the only issue is that can arise is that ffi bindings can used deprecated enum values.

This revision now requires changes to proceed.Apr 13 2016, 11:22 PM

http://llvm.org/docs/LangRef.html#parameter-attributes and http://llvm.org/docs/LangRef.html#function-attributes will give you all the attributes. I don't see how duplicating what is already in there makes any sense.

rafael edited edge metadata.Apr 14 2016, 5:48 AM
rafael added a subscriber: rafael.

Note that name aren't more stable than ids in theory. in practice they are much more stable and it is possible for the function to return an error code on missing attribute name as suggested. by @whitequark .

I am fine with this as long as we document that an attribute name
might go away without the usual deprecation period that other C
visible features get.

Cheers,
Rafael

whitequark edited edge metadata.Apr 14 2016, 8:38 AM

Will target dependent attributes be covered by the API (like "no-sse")? Or is it only the regular attributes?

Well they really should be.

The advantage of enums is that they are self documenting, the disadvantage is that ffi bindings can get stale enums. (if we have rules that we can't reuse old values) the only issue is that can arise is that ffi bindings can used deprecated enum values.

Not just that. In practice, bindings don't bother exporting the full list of attributes, and neither do the people who add attributes bother to update the C API, and this causes a lot of pain when writing frontends that need a new attribute because I need at least two new patches, temporarily fork LLVM and LLVM bindings, etc. Enums are just bad for things that change at all frequently. They're ugly enough for ValueKind, which changes far less.

Will target dependent attributes be covered by the API (like "no-sse")? Or is it only the regular attributes?

Well they really should be.

I don't think this is in the scope of this patch. These are different kind of attributes. There is already some sparse support for these in the C API with functions like "LLVMAddTargetDependentFunctionAttr" but support is sparse.

deadalnix updated this revision to Diff 53818.Apr 14 2016, 5:07 PM
deadalnix edited edge metadata.

Add release not and doc block for the function mentionning that attribute names and ids can change over time.

deadalnix updated this revision to Diff 53821.Apr 14 2016, 5:14 PM
deadalnix edited edge metadata.

Update disclaimer

So, the problem this is solving isn't that we need to be able to dynamically assign attribute ids, but that the C api uses a bitmask for attributes, rather than identifying them by, say, small ints, and thus it runs out of space.

But, what is the plan withLLVMGetAttribute (and similar) functions which cannot work if LLVMAttribute isn't a bitmap, since it returns just a single value?

There's also further issues which perhaps should be considered as part of a revamp of the C attribute access APIs: attributes aren't actually a simple flag, but can also have values. Align is already handled via extra functions in the C API, but there's a bunch of others now, too. Should all of those also get their own functions added? The "Target-dependent" attributes are one special case of that -- arbitrary string attribute names and values -- but the C API currently only supports setting them not retrieving them, and only on functions, not parameters.

I'd really like to see a more fleshed-out proposal for what the final state is expected to be.

@jyknight let's keep the target dependent attributes out of this. They have a different API in C as in C++ so it is a separate problem.

For attribute with values, how many of them exists ? I was under the impression that going the align road with them was fine (ie having special function for these).

The plan with LLVMGetAttribute is to deprecate it in favor of LLVMHasAttribute (for function, args/params and return values). This part is actually quite straightforward.

Wallbraker requested changes to this revision.Apr 17 2016, 6:12 AM
Wallbraker edited edge metadata.

Thanks

Maybe not the links but put in the comment that its function/arg/return attributes that this functions returns, and mention that they are listed in the language reference.

The C/C++ has crap documentation, lets try not to add more, and people will not find "documentation" in the commit message or reviews.

Cheers, Jakob.

This revision now requires changes to proceed.Apr 17 2016, 6:12 AM
deadalnix updated this revision to Diff 54136.Apr 18 2016, 3:54 PM
deadalnix edited edge metadata.

Mention that this is for builtin attributes

deadalnix updated this revision to Diff 54137.Apr 18 2016, 3:55 PM
deadalnix edited edge metadata.

aka "target independent"

deadalnix updated this revision to Diff 54140.Apr 18 2016, 4:47 PM

Add links

Wallbraker accepted this revision.Apr 19 2016, 2:35 PM
Wallbraker edited edge metadata.

Looks good, thanks!

This revision is now accepted and ready to land.Apr 19 2016, 2:35 PM
This revision was automatically updated to reflect the committed changes.