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

[deadalnix]

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.

[deadalnix]

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]

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.

[deadalnix]

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]

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]

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.

[deadalnix]

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]

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

[deadalnix]

Update disclaimer

[jyknight]

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.

[deadalnix]

@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]

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.

[deadalnix]

Mention that this is for builtin attributes

[deadalnix]

aka "target independent"

[deadalnix]

Add links

[Wallbraker]

Looks good, thanks!

docs/ReleaseNotes.rst

 
 * The C API function LLVMLinkModules has been removed.
 
 * The C API function LLVMAddTargetData has been removed.
 
 * The C API function LLVMGetDataLayout is deprecated
   in favor of LLVMGetDataLayoutStr.
 
+* The C API enum LLVMAttribute is deprecated in favor of
+  LLVMGetAttrKindID.
+
 * ``TargetFrameLowering::eliminateCallFramePseudoInstr`` now returns an
   iterator to the next instruction instead of ``void``. Targets that previously
   did ``MBB.erase(I); return;`` now probably want ``return MBB.erase(I);``.
 
 .. NOTE
    For small 1-3 sentence descriptions, just add an entry at the end of
    this list. If your description won't fit comfortably in one bullet
    point (e.g. maybe you would like to give an example of the

include/llvm-c/Core.h

  */
 LLVMDiagnosticSeverity LLVMGetDiagInfoSeverity(LLVMDiagnosticInfoRef DI);
 
 unsigned LLVMGetMDKindIDInContext(LLVMContextRef C, const char *Name,
                                   unsigned SLen);
 unsigned LLVMGetMDKindID(const char *Name, unsigned SLen);
 
 /**
+ * Return an unique id given an attribute name, or 0 if no attribute
+ * by that name exists.
+ *
+ * NB: Attribute names and/or id are subject to change over time.
+ */
+unsigned LLVMGetAttrKindID(const char *Name, size_t SLen);
+
+/**
  * @}
  */
 
 /**
  * @defgroup LLVMCCoreModule Modules
  *
  * Modules represent the top-level structure in an LLVM program. An LLVM
  * module is effectively a translation unit or a collection of

lib/IR/Core.cpp

 //===-- Core.cpp ----------------------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the common infrastructure (including the C bindings)
 // for libLLVMCore.a, which implements the LLVM intermediate representation.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm-c/Core.h"
+#include "llvm/ADT/StringSwitch.h"
+#include "AttributeImpl.h"
 #include "llvm/Bitcode/ReaderWriter.h"
 #include "llvm/IR/Attributes.h"
 #include "llvm/IR/CallSite.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/DiagnosticPrinter.h"
 #include "llvm/IR/GlobalAlias.h"
unsigned LLVMGetMDKindIDInContext(LLVMContextRef C, const char *Name,
                                   unsigned SLen) {
   return unwrap(C)->getMDKindID(StringRef(Name, SLen));
 }
 
 unsigned LLVMGetMDKindID(const char *Name, unsigned SLen) {
   return LLVMGetMDKindIDInContext(LLVMGetGlobalContext(), Name, SLen);
 }
 
+#define GET_ATTR_KIND_FROM_NAME
+#include "AttributesCompatFunc.inc"
+
+unsigned LLVMGetAttrKindID(const char *Name, size_t SLen) {
+  auto K = getAttrKindFromName(StringRef(Name, SLen));
+  if (K == Attribute::None) {
+    return 0;
+  }
+
+  return AttributeImpl::getAttrMask(K);
+}
+
 char *LLVMGetDiagInfoDescription(LLVMDiagnosticInfoRef DI) {
   std::string MsgStorage;
   raw_string_ostream Stream(MsgStorage);
   DiagnosticPrinterRawOStream DP(Stream);
 
   unwrap(DI)->print(DP);
   Stream.flush();
 

utils/TableGen/Attributes.cpp

 
 class Attributes {
 public:
   Attributes(RecordKeeper &R) : Records(R) {}
   void emit(raw_ostream &OS);
 
 private:
   void emitTargetIndependentEnums(raw_ostream &OS);
+  void emitConversionFn(raw_ostream &OS);
   void emitFnAttrCompatCheck(raw_ostream &OS, bool IsStringAttr);
 
   void printEnumAttrClasses(raw_ostream &OS,
                             const std::vector<Record *> &Records);
   void printStrBoolAttrClasses(raw_ostream &OS,
                                const std::vector<Record *> &Records);
 
   RecordKeeper &Records;
std::vector<Record*> Attrs =
       Records.getAllDerivedDefinitions("EnumAttr");
 
   for (auto A : Attrs)
     OS << A->getName() << ",\n";
 
   OS << "#endif\n";
 }
 
+void Attributes::emitConversionFn(raw_ostream &OS) {
+  OS << "#ifdef GET_ATTR_KIND_FROM_NAME\n";
+  OS << "#undef GET_ATTR_KIND_FROM_NAME\n";
+
+  std::vector<Record*> Attrs =
+      Records.getAllDerivedDefinitions("EnumAttr");
+
+  OS << "static Attribute::AttrKind getAttrKindFromName(StringRef AttrName) {\n";
+  OS << "  return StringSwitch<Attribute::AttrKind>(AttrName)\n";
+
+  for (auto A : Attrs) {
+    OS << "    .Case(\"" << A->getValueAsString("AttrString");
+    OS << "\", Attribute::" << A->getName() << ")\n";
+  }
+
+  OS << "    .Default(Attribute::None);\n";
+  OS << "}\n\n";
+
+  OS << "#endif\n";
+}
+
 void Attributes::emitFnAttrCompatCheck(raw_ostream &OS, bool IsStringAttr) {
   OS << "#ifdef GET_ATTR_COMPAT_FUNC\n";
   OS << "#undef GET_ATTR_COMPAT_FUNC\n";
 
   OS << "struct EnumAttr {\n";
   OS << "  static bool isSet(const Function &Fn,\n";
   OS << "                    Attribute::AttrKind Kind) {\n";
   OS << "    return Fn.hasFnAttribute(Kind);\n";
for (const auto *R : Records) {
     OS << "  }\n";
     OS << "};\n";
   }
   OS << "\n";
 }
 
 void Attributes::emit(raw_ostream &OS) {
   emitTargetIndependentEnums(OS);
+  emitConversionFn(OS);
   emitFnAttrCompatCheck(OS, false);
 }
 
 namespace llvm {
 
 void EmitAttributes(RecordKeeper &RK, raw_ostream &OS) {
   Attributes(RK).emit(OS);
 }
 
 } // End llvm namespace.