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.

[whitequark]

include/llvm-c/Core.h

438 ↗

(On Diff #52518)

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

[deadalnix]

Use size_t

[whitequark]

include/llvm-c/Core.h

438 ↗

(On Diff #52523)

Header doesn't match source.

[deadalnix]

Messed up adding size_t . Now size_t actually added.

[whitequark]

include/llvm-c/Core.h

439 ↗

(On Diff #52524)

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

lib/IR/Core.cpp

119 ↗

(On Diff #52524)

getAttrKindFromName

121 ↗

(On Diff #52524)

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]

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

[deadalnix]

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

[deadalnix]

lib/IR/Core.cpp

121 ↗

(On Diff #52524)

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]

getAttrKindFromName

[deadalnix]

Remove non in context verison of the function

[Wallbraker]

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?

[deadalnix]

Ping !

[whitequark]

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

[• 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

[whitequark]

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

[deadalnix]

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

[whitequark]

@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).

[Wallbraker]

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.

[deadalnix]

@Wallbraker > http://reviews.llvm.org/D19081

llvm/trunk/include/llvm-c/Core.h

  * @see DiagnosticInfo::getSeverity()
  */
 LLVMDiagnosticSeverity LLVMGetDiagInfoSeverity(LLVMDiagnosticInfoRef DI);
 
 unsigned LLVMGetMDKindIDInContext(LLVMContextRef C, const char *Name,
                                   unsigned SLen);
 unsigned LLVMGetMDKindID(const char *Name, unsigned SLen);
 
+unsigned LLVMGetAttrKindIDInContext(LLVMContextRef C, const char *Name,
+                                    size_t SLen);
+
 /**
  * @}
  */
 
 /**
  * @defgroup LLVMCCoreModule Modules
  *
  * Modules represent the top-level structure in an LLVM program. An LLVM

llvm/trunk/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 LLVMGetAttrKindIDInContext(LLVMContextRef C, const char *Name,
+                                    size_t SLen) {
+  auto K = getAttrKindFromName(StringRef(Name, SLen));
+  assert(K != Attribute::None && "Invalid attribute");
+  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();
 

llvm/trunk/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.