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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/include/llvm-c/lto2.h:123
+/// Retrieve the name for a symbol.
+LTOErrorStr lto_symbol_name(lto_symbol_t);
+
----------------
mehdi_amini wrote:
> 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).
Yes, the allocation could be a problem if it were the responsibility of the API. But another possibility would be for the client to allocate the data structure and have the API fill it in.

The client side would look like this:
```
lto_symbol_attr attrs[2];
attrs[0].tag = tag_name;
attrs[1].tag = tag_visibility;

for (int i = 0; i != n; ++i) {
  lto_get_symbol_attrs(input_file, i, attrs, sizeof(attrs));
  char *name = attrs[0].value.u_ptr;
  lto_visibility vis = attrs[1].value.u_int;
  // process the symbol
}
```
What do you think?


================
Comment at: llvm/include/llvm-c/lto2.h:133
+  size_t NumSyms;
+} LTOSymbols;
+
----------------
mehdi_amini wrote:
> 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.
Right, I expect that we will continue to provide random access via the irsymtab (or whatever we replace it with).


================
Comment at: llvm/lib/LTO/LTO.cpp:527
+    if (ResI == ResE)
+      return make_error<StringError>("List of resolution is too short",
+                                     std::error_code());
----------------
I think you can move this check into lto2.cpp now that the symbol list is random access.


https://reviews.llvm.org/D31898