[PATCH] D31898: Introduce libLTO C APIs to target the "resolution-based" new LTO API

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

A few comments, mostly on the interface.



================
Comment at: llvm/include/llvm-c/lto2.h:52
+
+/// Error handling for these API: they all returned a string in case or error,
+/// or a nullptr on success. The string is mallocated and has to be free'd by
----------------
What is the minimum level of C we support in our C interfaces? If it is C89 we should use `/* */` in this file.


================
Comment at: llvm/include/llvm-c/lto2.h:55
+/// the client.
+typedef const char *LTOErrorStr;
+
----------------
Please be more consistent about type naming (i.e. `lto_foo` vs `LTOFoo`).


================
Comment at: llvm/include/llvm-c/lto2.h:123
+/// Retrieve the name for a symbol.
+LTOErrorStr lto_symbol_name(lto_symbol_t);
+
----------------
How are you planning to expose other symbol attributes? I'd use an array-of-tagged-unions approach for that as well.


================
Comment at: llvm/include/llvm-c/lto2.h:133
+  size_t NumSyms;
+} LTOSymbols;
+
----------------
Is this data structure needed? I think it would be sufficient to just have a way of querying for the number of symbols and symbol attributes for the `n`th symbol given an `lto_input_t`.


================
Comment at: llvm/include/llvm-c/lto2.h:161
+/** FIXME: when? */
+void lto_destroy_input(lto_input_t input);
+
----------------
I would imagine that a client would use this function to destroy the input file after retrieving the list of symbols without adding it to an LTO object, for example if the client is `ar` or wants to support gold's `--start-lib`/`--end-lib` flags.


================
Comment at: llvm/include/llvm-c/lto2.h:184
+typedef struct {
+  lto_symbol_resolution_t *resolutions;
+  size_t num;
----------------
Maybe there should be a tag field in `lto_symbol_resolutions_t` so that we can introduce new versions of the `lto_symbol_resolution_t` data structure by incrementing the tag.


================
Comment at: llvm/include/llvm-c/lto2.h:189
+/** Add an input to the LTO instance */
+LTOErrorStr lto_add_input(lto_instance_t, lto_input_t,
+                          lto_symbol_resolutions_t);
----------------
Document that this function takes ownership of the `lto_input_t`.


================
Comment at: llvm/include/llvm/LTO/LTO.h:125-128
+    using irsymtab::Symbol::isUsed;
+    using irsymtab::Symbol::isGlobal;
+    using irsymtab::Symbol::isFormatSpecific;
+    using irsymtab::Symbol::isUnnamedAddr;
----------------
Is this change necessary?


================
Comment at: llvm/tools/lto/lto2.cpp:110
+extern "C" const char *lto_symbol_name(lto_symbol_t Sym) {
+  return unwrap(Sym)->getName().data();
+}
----------------
This isn't guaranteed to be null terminated (especially after the string table changes), so this probably needs to return a pointer and a length.


https://reviews.llvm.org/D31898