[PATCH] D60974: Clang IFSO driver action.

Puyan Lotfi via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon May 6 10:04:52 PDT 2019


plotfi added a comment.

In D60974#1490358 <https://reviews.llvm.org/D60974#1490358>, @rupprecht wrote:

> I didn't follow the technical details, but I don't see anything wrong with moving forward on this patch. I think this seems like an interesting idea worth experimenting with.
>
> In D60974#1488563 <https://reviews.llvm.org/D60974#1488563>, @jakehehrlich wrote:
>
> > > Jake, I am still not sure what you would prefer for moving forward. The current patch can output the yaml format I originally proposed or the one that looks similar to the one you proposed. What I need to know is:
> > > 
> > > Do you want the merging if the "ifo" files to happen inside of llvm-elfabi?
> > >  Do you care if we upstream the yaml we proposed as a first step (which is really a distilled version of that yaml2obj can consume anyways. this right now functions actually) ???
> > >  Or, would you rather the ifo files be merged by a different separate tool (something maybe called llvm-ifsogen)???
> >
> > This is my proposal:
> >
> > 1. We should plan on adding the code ofr merging these files inside of `llvm/tools/llvm-elfabi` but will it be a separate tool from `llvm-elfabi`. You can create "separate" tools in the same directory using the symlink trick. See llvm-ar and friends or llvm-objcopy and llvm-strip. The tool name can be discussed in review.
>
>
> The symlink trick is usually used when the new frontend tool is just a thin layer over the underlying tool. Is that going to be the case here?
>
> For example,
>
> - `llvm-ranlib` is equivalent to `llvm-ar s`
> - `llvm-readelf` is equivalent to `llvm-readobj --elf-output-style=GNU`
> - `llvm-addr2line` is equivalent to `llvm-symbolizer --output-style=GNU`
> - `llvm-strip` is equivalent to `llvm-objcopy --strip-all` (maybe not exactly)
>
>   In other words, is `llvm-mergeifo` (placeholder name) going to be roughly equivalent to `llvm-elfabi --merge-ifo` (again, placeholder flag name)? And if so, it doesn't seem like a symlink is strictly necessary -- users can just call `llvm-elfabi --merge-ifo`. Am I missing something?


@rupprecht
I was looking into this. I think it could work to just have a secondary path for loading tbe files and merging them. I modified the tbe schema to include an "Endian" field (which I would like to emit from clang).

>> 2. The yaml proposed thus far is not acceptable IMO for reasons already outlined but this can be discussed in code review. Before adding sections a clear reason for needing them is required which hasn't been given yet. Things can evolve and change later as needed but we should start with the minimum and add as needed later; not start with extra and remove later. Support for the new format should be added into TextAPI and in the review for that process we should discuss the format. After we add support for this new format into TextAPI we can add support for emitting this format into clang (well actually you can write the code whenever but I'm using "after" in a different sense here).
> 
> What if we named it experimental for now? i.e. `--experimental-emit-ifo` for the clang flag name and `!experimental-ifo-elf-v1` for the yaml id? That would allow those working on this patch to play around with more features, but still give sufficient warning to anyone that fields they depend on may be removed.
> 
> In fact, if we label it experimental (or maybe even if we don't), I don't see any reason this couldn't land now, even without a consumer of it. So what if a tool produces a yaml file that we don't haven't finished the yaml parser for? Does anything break?

@rupprecht I like this idea a lot, and I think it's a good compromise. I think in the long run there are multiple formats that could be output from clang-emit-interface-stubs so I went ahead and added an additional -interface-stubs-version= flag (because why limit the output format to a specific version of tbe or elf?). My current diff can generate either tbe (that can be consumed by llvm-elfabi) by default or yaml that can be consumed by yaml2obj (when passing -interface-stubs-version=experimental-ifo-elf-v1). The default output pretty much produces the exact tbe that llvm-elfabi consumes so its not really a new yaml schema here (-interface-stubs-version=tapi-tbe or the default will do this), on the other hand -interface-stubs-version=experimental-ifo-elf-v1 can be used to give us freedom to target the internal yaml2obj tool if we needed to. I still need to go back and change all the references to either "ifso" or "ifo" to "interface stub" or "ifs" for consistency. I hope this is a reasonable compromise and can land with close to what I have here. @jakehehrlich  @compnerd thoughts?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974





More information about the cfe-commits mailing list