[llvm-c] Improve IR Introspection: Add LLVM{Get,Set}ModuleIdentifier

[• ubsan]

Summary

Also, tests and stuff

[whitequark]

tools/llvm-c-test/echo.cpp

221

Why is this in clone_constant? It should be in llvm_echo below (and only executed once...)

[Wallbraker]

include/llvm-c/Core.h

483–504

Please add a length parameter for both functions. Not all languages keep their string zero terminated and keep track of the length. For C StringRef will call strlen so the cost is the same.

char *LLVMGetModuleIdentifier(LLVMModuleRef M, size_t *Length);
void LLVMSetModuleIdentifier(LLVMModuleRef M, const char *ID, size_t Length);

[whitequark]

include/llvm-c/Core.h

483–504

No other function in llvm-c currently does this. I don't see how the nonexistent benefit of being able to add a non-zero-terminated string for a human-readable identifier overweighs the drawback of making the API inconsistent.

[Wallbraker]

include/llvm-c/Core.h

483–504

Nitpicking: LLVMConstString* functions does include a Length argument, as does the new functions in D18727.

I have to agree it would be inconsistent, I will retract my comment and refer the issue to other stake holders in the C API.

[deadalnix]

include/llvm-c/Core.h

483–504

Actually, several function in C API do that (for instance LLVMGetAsString and LLVMGetMDString), while other use zero terminated strings. The current state is rather inconsistent. If we are going to make it consistent, I'd go for the Length as out parameter.

[deadalnix]

tools/llvm-c-test/echo.cpp

221

This has nothing to do here.

[whitequark]

include/llvm-c/Core.h

483–504

That's reasonable. Nicole, can you please change the API as suggested?

[• ubsan]

include/llvm-c/Core.h

483–504

So, both zero terminated string and string with length as out param? Or just the latter?

[whitequark]

include/llvm-c/Core.h

483–504

No, just the latter, it doesn't make a lot of sense to have both versions of this API when we agree that the string+length variant is the way it should all look eventually.

[• ubsan]

include/llvm-c/Core.h

483–504

Great, will change.

[• ubsan]

include/llvm-c/Core.h

483–504

Should it take an unsigned, or a size_t? A size_t would be more in keeping with what is actually meant by "length", but unsigned is more consistent with the rest of llvm.

[deadalnix]

include/llvm-c/Core.h

483–504

I think you are right, size_t is the correct type here, while I don't think it makes a big difference in practice (I don't expect anyone to get anywhere near 4G of description for a module).

[deadalnix]

lib/IR/Core.cpp

161–162

A string should know its length already, so taking the pointer and then computing strlen is wasteful. It is also not needed to strdup. It is common practice in the C API to return string the caller don't have ownership of. If caller has ownership of something, then a LLVMDisposeXXX method is provided. In this case, I don't think this is necessary.

auto &Str = unwrap(M)->getModuleIdentifier()
*Length = Str.length();
return Str.c_str();

Also, just take a size_t for length so yu don't need to assert. I see no reason why someone would have such a large identifier that it matters, but, on the other hand, I see no reason to prevent it.

[deadalnix]

tools/llvm-c-test/echo.cpp

880

What happens if you read the module identifier from src and copy it to dst ? That would avoid having to hardcode a module name as it is done now.

[• ubsan]

tools/llvm-c-test/echo.cpp

880

How do you mean?

[• ubsan]

include/llvm-c/Core.h

483–504

However, everything else uses unsigned for length.

lib/IR/Core.cpp

161–162

Will do.

[• ubsan]

Return a const string instead.

[deadalnix]

lib/IR/Core.cpp

161–162

As said, just use size_t and get rid of the assert.

tools/llvm-c-test/echo.cpp

880

This test take a module as input, read it, and write a copy in another module. That allow to test read and write operations.

Reading the identifier from the Src module writing it in the Dst module seems like the way to do that.

[• ubsan]

Follow deadalnix's suggestions.

[whitequark]

lib/IR/Core.cpp

167

Actually, can you call StringRef constructor here? I'm pretty sure I've never seen brace-initializer style anywhere else in LLVM.

[• ubsan]

lib/IR/Core.cpp

167

Yeah, I can. I will change it when I can.

[• ubsan]

Switch to explicit constructor.

[Wallbraker]

Looks good, thanks for all the work!

include/llvm-c/Core.h

 /**
  * Destroy a module instance.
  *
  * This must be called for every created module or memory will be
  * leaked.
  */
 void LLVMDisposeModule(LLVMModuleRef M);
 
 /**
+ * Obtain the identifier of a module.
+ *
+ * @param M Module to obtain identifier of
+ * @param Len Out parameter which holds the length of the returned string.
+ * @return The identifier of M.
+ * @see Module::getModuleIdentifier()
+ */
+const char *LLVMGetModuleIdentifier(LLVMModuleRef M, size_t *Len);
+
+/**
+ * Set the identifier of a module to a string Ident with length Len.
+ *
+ * @param M The module to set identifier
+ * @param Ident The string to set M's identifier to
+ * @param Len Length of Ident
+ * @see Module::setModuleIdentifier()
+ */
+void LLVMSetModuleIdentifier(LLVMModuleRef M, const char *Ident, size_t Len);
+
+/**
  * Obtain the data layout for a module.
  *
  * @see Module::getDataLayoutStr()
  *
  * LLVMGetDataLayout is DEPRECATED, as the name is not only incorrect,
  * but match the name of another method on the module. Prefer the use
  * of LLVMGetDataLayoutStr, which is not ambiguous.
  */
 const char *LLVMGetDataLayoutStr(LLVMModuleRef M);

lib/IR/Core.cpp

LLVMModuleRef LLVMModuleCreateWithNameInContext(const char *ModuleID,
                                                 LLVMContextRef C) {
   return wrap(new Module(ModuleID, *unwrap(C)));
 }
 
 void LLVMDisposeModule(LLVMModuleRef M) {
   delete unwrap(M);
 }
 
+const char *LLVMGetModuleIdentifier(LLVMModuleRef M, size_t *Len) {
+  auto &Str = unwrap(M)->getModuleIdentifier();
+  *Len = Str.length();
+  return Str.c_str();
+}
+
+void LLVMSetModuleIdentifier(LLVMModuleRef M, const char *Ident, size_t Len) {
+  unwrap(M)->setModuleIdentifier({Ident, Len});
+}
+
+
 /*--.. Data layout .........................................................--*/
 const char *LLVMGetDataLayoutStr(LLVMModuleRef M) {
   return unwrap(M)->getDataLayoutStr().c_str();
 }
 
 const char *LLVMGetDataLayout(LLVMModuleRef M) {
   return LLVMGetDataLayoutStr(M);
 }

tools/llvm-c-test/echo.cpp

 }
 
 LLVMValueRef clone_constant(LLVMValueRef Cst, LLVMModuleRef M) {
   if (!LLVMIsAConstant(Cst))
     report_fatal_error("Expected a constant");
 
   // Maybe it is a symbol
   if (LLVMIsAGlobalValue(Cst)) {
     const char *Name = LLVMGetValueName(Cst);
 
     // Try function
     if (LLVMIsAFunction(Cst)) {
       LLVMValueRef Dst = LLVMGetNamedFunction(M, Name);
       if (Dst)
         return Dst;
       report_fatal_error("Could not find function");
     }
while (true) {
     Cur = Next;
   }
 }
 
 int llvm_echo(void) {
   LLVMEnablePrettyStackTrace();
 
   LLVMModuleRef Src = llvm_load_module(false, true);
-
+  size_t Len;
+  const char *ModuleName = LLVMGetModuleIdentifier(Src, &Len);
   LLVMContextRef Ctx = LLVMContextCreate();
-  LLVMModuleRef M = LLVMModuleCreateWithNameInContext("<stdin>", Ctx);
+  LLVMModuleRef M = LLVMModuleCreateWithNameInContext(ModuleName, Ctx);
+
+  // This whole switcharound is done because the C API has no way to
+  // set the source_filename
+  LLVMSetModuleIdentifier(M, "", 0);
+  LLVMGetModuleIdentifier(M, &Len);
+  if (Len != 0)
+      report_fatal_error("LLVM{Set,Get}ModuleIdentifier failed");
+  LLVMSetModuleIdentifier(M, ModuleName, strlen(ModuleName));
 
   LLVMSetTarget(M, LLVMGetTarget(Src));
   LLVMSetModuleDataLayout(M, LLVMGetModuleDataLayout(Src));
   if (strcmp(LLVMGetDataLayoutStr(M), LLVMGetDataLayoutStr(Src)))
     report_fatal_error("Inconsistent DataLayout string representation");
 
   declare_symbols(Src, M);
   clone_symbols(Src, M);
   char *Str = LLVMPrintModuleToString(M);
   fputs(Str, stdout);
 
   LLVMDisposeMessage(Str);
   LLVMDisposeModule(M);
   LLVMContextDispose(Ctx);
 
   return 0;
 }