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

[• ubsan]

Summary

Also, tests and stuff

[whitequark]

tools/llvm-c-test/echo.cpp

221 ↗

(On Diff #52490)

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

[Wallbraker]

include/llvm-c/Core.h

483–496 ↗

(On Diff #52490)

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–496 ↗

(On Diff #52490)

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–496 ↗

(On Diff #52490)

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–496 ↗

(On Diff #52490)

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 ↗

(On Diff #52490)

This has nothing to do here.

[whitequark]

include/llvm-c/Core.h

483–496 ↗

(On Diff #52490)

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

[• ubsan]

include/llvm-c/Core.h

483–496 ↗

(On Diff #52490)

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

[whitequark]

include/llvm-c/Core.h

483–496 ↗

(On Diff #52490)

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–496 ↗

(On Diff #52490)

Great, will change.

[• ubsan]

include/llvm-c/Core.h

483–496 ↗

(On Diff #52490)

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–505 ↗

(On Diff #52517)

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 ↗

(On Diff #52517)

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

881 ↗

(On Diff #52517)

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

881 ↗

(On Diff #52517)

How do you mean?

[• ubsan]

include/llvm-c/Core.h

483–505 ↗

(On Diff #52517)

However, everything else uses unsigned for length.

lib/IR/Core.cpp

161–162 ↗

(On Diff #52517)

Will do.

[• ubsan]

Return a const string instead.

[deadalnix]

lib/IR/Core.cpp

161–162 ↗

(On Diff #52520)

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

tools/llvm-c-test/echo.cpp

881 ↗

(On Diff #52520)

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 ↗

(On Diff #52576)

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 ↗

(On Diff #52576)

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

[• ubsan]

Switch to explicit constructor.

[Wallbraker]

Looks good, thanks for all the work!

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

llvm/trunk/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(StringRef(Ident, Len));
+}
+
+
 /*--.. Data layout .........................................................--*/
 const char *LLVMGetDataLayoutStr(LLVMModuleRef M) {
   return unwrap(M)->getDataLayoutStr().c_str();
 }
 
 const char *LLVMGetDataLayout(LLVMModuleRef M) {
   return LLVMGetDataLayoutStr(M);
 }

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

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);