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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini 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);
+
----------------
pcc wrote:
> 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?
That looks nicer this way.

That said I forgot to mention that I planned to also have a c++ wrapper for the C API, so that the client could have a C++-like API that would target the C API exposed by LTO.
Something like:

```
namespace {
class LTOSymbol {
  lto_symbol_t Sym;
public:
  LTOStringRef getName() {
    return lto_symbol_name(Sym);
  }
};
}
```

But also something that would allow to write:

```
LTOInput Input = ...;
for (LTOSymbol Sym : Input) {
  auto Name = Sym.getName();
  ...
}
```

So I'd like to have an API that allows to write a nice wrapper for the client.


================
Comment at: llvm/include/llvm-c/lto2.h:207
+ * supposed to be called once per instance. The instance can't be reused and
+ * should be destroyed after this call returns.
+ */
----------------
Wallbraker wrote:
> Why not just destroy it automatically?
Not auto-destroying leaves the possibility to introduce new APIs that can be called after lto_run without breaking existing client.


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


https://reviews.llvm.org/D31898