[llvm-c] Make LLVMAddAlias opaque pointer compatible

[nikic]

Summary

Deprecate LLVMAddAlias in favor of LLVMAddAlias2, which accepts a value type and an address space. Previously these were extracted from the pointer type.

[nikic]

I'd also like some advice on deprecations in the C API: As this is an FFI API, marking the functions as deprecated in the header is of limited usefulness, and we currently don't do that. But this means that people using the FFI API are likely not aware they need to take action wrt opaque pointers. I would propose that we go ahead and delete old deprecated functions like LLVMBuildInvoke, so that people are forced to take action now, while easy workarounds (like LLVMGetElementType) are still available.

The only alternative that comes to mind is a link-time deprecation, but I'm not familiar with how these are done and how well they work across platforms.

[aeubanks]

I think deleting functions with proper alternatives is fine, probably one at a time though so people can work through updating calls to those functions.

[dblaikie]

In D114860#3164905, @aeubanks wrote:

I think deleting functions with proper alternatives is fine, probably one at a time though so people can work through updating calls to those functions.

Even if there's no way to communicate to all users that the original is deprecated (because they might be calling it through FFI in another language that doesn't consume the C header) there's probably still some amount of C header usage - so I think marking it as deprecated for a release & documenting it in the release notes, then removing it, seems suitable. Gives people at least /some/ chance to migrate on their own timeline rather than making it a hard break from the moment the new API is introduced with no chance of decoupling the transition. (also means even if you don't find out about the issue until you upgrade to a version missing the API - you can do a patch to your mainline that adopts the new API prior to picking up the breaking version - even if it's just minutes/hours/etc it decouples the release process a bit/doesn't make it such a monolithic change, which is good)

[nikic]

@dblaikie To be clear, I'm not talking about the API replaced here, but about APIs that had an opaque pointer compatible version for many years now. I agree that there should be time between adding the replacement API and removing the old one.

[CodaFi]

I think deleting functions with proper alternatives is fine, probably one at a time though so people can work through updating calls to those functions.

This is not, and has never been, the policy for deprecation for C API functions. We have a strict policy of ABI compatibility (for better or for worse) in these C bindings, these functions must stay.

Now, am I saying this is a good thing: absolutely not. I think this policy makes very little sense and we ought to get it changed. There's just not much will to do so at the moment.

[dblaikie]

In D114860#3165439, @CodaFi wrote:

I think deleting functions with proper alternatives is fine, probably one at a time though so people can work through updating calls to those functions.

This is not, and has never been, the policy for deprecation for C API functions. We have a strict policy of ABI compatibility (for better or for worse) in these C bindings, these functions must stay.

Now, am I saying this is a good thing: absolutely not. I think this policy makes very little sense and we ought to get it changed. There's just not much will to do so at the moment.

Like, at some point, these functions are going to be removed because it won't be possible to express them anymore - pointer types won't carry the necessary information. So it's not a question of if, but when.

In D114860#3165103, @nikic wrote:

@dblaikie To be clear, I'm not talking about the API replaced here, but about APIs that had an opaque pointer compatible version for many years now. I agree that there should be time between adding the replacement API and removing the old one.

Ah, sure - if something's been attributed as deprecated with a new alternative and had at least a release and it's going to be removed eventually, I'm OK with it being removed then. (I wouldn't argue that it /has/ to be attributed as deprecated for at least a release - if it's just in the release notes but doesn't have the attribute, someone could twist my arm into convincing me that it's still had enough opportunity for migration - but I'd prefer the attribute to go in whenever the new alternative API goes in anyway)

[dexonsmith]

In D114860#3165439, @CodaFi wrote:

I think deleting functions with proper alternatives is fine, probably one at a time though so people can work through updating calls to those functions.

This is not, and has never been, the policy for deprecation for C API functions. We have a strict policy of ABI compatibility (for better or for worse) in these C bindings, these functions must stay.

That strict policy changed a few years ago based on an llvm-dev discussion, whose result was documented in d9f8ce997773e18315e04ab17d052f1db926d77b / r255300. The policy since then has been "best effort". The current text is:
https://llvm.org/docs/DeveloperPolicy.html#c-api-changes

[nikic]

I submitted D114936 for header deprecation support in the C API. I think it won't help most consumers of the C API, but it definitely shouldn't hurt at least :)

llvm/include/llvm-c/Core.h

  * @defgroup LLVMCoreValueConstantGlobalAlias Global Aliases
  *
  * This group contains function that operate on global alias values.
  *
  * @see llvm::GlobalAlias
  *
  * @{
  */
+
+/** Deprecated: Use LLVMAddAlias2 instead. */
 LLVMValueRef LLVMAddAlias(LLVMModuleRef M, LLVMTypeRef Ty, LLVMValueRef Aliasee,
                           const char *Name);
 
 /**
+ * Add a GlobalAlias with the given value type, address space and aliasee.
+ *
+ * @see llvm::GlobalAlias::create()
+ */
+LLVMValueRef LLVMAddAlias2(LLVMModuleRef M, LLVMTypeRef ValueTy,
+                           unsigned AddrSpace, LLVMValueRef Aliasee,
+                           const char *Name);
+
+/**
  * Obtain a GlobalAlias value from a Module by its name.
  *
  * The returned value corresponds to a llvm::GlobalAlias value.
  *
  * @see llvm::Module::getNamedAlias()
  */
 LLVMValueRef LLVMGetNamedGlobalAlias(LLVMModuleRef M,
                                      const char *Name, size_t NameLen);

llvm/lib/IR/Core.cpp

 LLVMValueRef LLVMAddAlias(LLVMModuleRef M, LLVMTypeRef Ty, LLVMValueRef Aliasee,
                           const char *Name) {
   auto *PTy = cast<PointerType>(unwrap(Ty));
   return wrap(GlobalAlias::create(PTy->getElementType(), PTy->getAddressSpace(),
                                   GlobalValue::ExternalLinkage, Name,
                                   unwrap<Constant>(Aliasee), unwrap(M)));
 }
 
+LLVMValueRef LLVMAddAlias2(LLVMModuleRef M, LLVMTypeRef ValueTy,
+                           unsigned AddrSpace, LLVMValueRef Aliasee,
+                           const char *Name) {
+  return wrap(GlobalAlias::create(unwrap(ValueTy), AddrSpace,
+                                  GlobalValue::ExternalLinkage, Name,
+                                  unwrap<Constant>(Aliasee), unwrap(M)));
+}
+
 LLVMValueRef LLVMGetNamedGlobalAlias(LLVMModuleRef M,
                                      const char *Name, size_t NameLen) {
   return wrap(unwrap(M)->getNamedAlias(Name));
 }
 
 LLVMValueRef LLVMGetFirstGlobalAlias(LLVMModuleRef M) {
   Module *Mod = unwrap(M);
   Module::alias_iterator I = Mod->alias_begin();

llvm/tools/llvm-c-test/echo.cpp

AliasDecl:
 
   Cur = Begin;
   Next = nullptr;
   while (true) {
     size_t NameLen;
     const char *Name = LLVMGetValueName2(Cur, &NameLen);
     if (LLVMGetNamedGlobalAlias(M, Name, NameLen))
       report_fatal_error("Global alias already cloned");
-    LLVMTypeRef CurType = TypeCloner(M).Clone(Cur);
+    LLVMTypeRef PtrType = TypeCloner(M).Clone(Cur);
+    LLVMTypeRef ValType = TypeCloner(M).Clone(LLVMGlobalGetValueType(Cur));
+    unsigned AddrSpace = LLVMGetPointerAddressSpace(PtrType);
     // FIXME: Allow NULL aliasee.
-    LLVMAddAlias(M, CurType, LLVMGetUndef(CurType), Name);
+    LLVMAddAlias2(M, ValType, AddrSpace, LLVMGetUndef(PtrType), Name);
 
     Next = LLVMGetNextGlobalAlias(Cur);
     if (Next == nullptr) {
       if (Cur != End)
         report_fatal_error("");
       break;
     }
 

llvm/unittests/IR/ConstantsTest.cpp

TEST(ConstantsTest, AliasCAPI) {
   SMDiagnostic Error;
   std::unique_ptr<Module> M =
       parseAssemblyString("@g = global i32 42", Error, Context);
   GlobalVariable *G = M->getGlobalVariable("g");
   Type *I16Ty = Type::getInt16Ty(Context);
   Type *I16PTy = PointerType::get(I16Ty, 0);
   Constant *Aliasee = ConstantExpr::getBitCast(G, I16PTy);
   LLVMValueRef AliasRef =
-      LLVMAddAlias(wrap(M.get()), wrap(I16PTy), wrap(Aliasee), "a");
+      LLVMAddAlias2(wrap(M.get()), wrap(I16Ty), 0, wrap(Aliasee), "a");
   ASSERT_EQ(unwrap<GlobalAlias>(AliasRef)->getAliasee(), Aliasee);
 }
 
 static std::string getNameOfType(Type *T) {
   std::string S;
   raw_string_ostream RSOS(S);
   T->print(RSOS);
   return S;