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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

Thanks, I'll address all these.



================
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
----------------
pcc wrote:
> What is the minimum level of C we support in our C interfaces? If it is C89 we should use `/* */` in this file.
Right, I planned to adopt the C like comments.


================
Comment at: llvm/include/llvm-c/lto2.h:55
+/// the client.
+typedef const char *LTOErrorStr;
+
----------------
pcc wrote:
> Please be more consistent about type naming (i.e. `lto_foo` vs `LTOFoo`).
That's another one that I noticed in the middle of the development: the existing LTO C API was likely based on ld64 convention, but I rather use the LLVM one. I'll likely update all the APIs as well.


================
Comment at: llvm/include/llvm-c/lto2.h:123
+/// Retrieve the name for a symbol.
+LTOErrorStr lto_symbol_name(lto_symbol_t);
+
----------------
pcc wrote:
> How are you planning to expose other symbol attributes? I'd use an array-of-tagged-unions approach for that as well.
I was planning to add a new API for each accessor. An array isn't great: it requires to allocate memory (which mean deallocating as well) and search in the array for the name when it is the only thing you want. I expect the client side to be more annoying to implement with an array.
(I may have misunderstood what you meant here).


================
Comment at: llvm/include/llvm-c/lto2.h:133
+  size_t NumSyms;
+} LTOSymbols;
+
----------------
pcc wrote:
> 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`.
I could do it now, I wrote this when the symbols were not backed by an array but some kind of list that we had to iterate through. So having an API with `get_ith_symbol(int n)` was requiring to iterate `n` times every call.
(we discuss it on IRC last month).

Now that you added the IR symtab, this does not add much value anymore.


================
Comment at: llvm/include/llvm-c/lto2.h:161
+/** FIXME: when? */
+void lto_destroy_input(lto_input_t input);
+
----------------
pcc wrote:
> 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.
Good point, I'll document this!


================
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;
----------------
pcc wrote:
> Is this change necessary?
No in this patch, at some point I was populating a special LTOSymbol structure containing all the information here. 


https://reviews.llvm.org/D31898