[PATCH] D31898: Introduce libLTO C APIs to target the "resolution-based" new LTO API
Peter Collingbourne via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Apr 10 12:02:35 PDT 2017
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
More information about the llvm-commits
mailing list