[PATCH] D60974: Clang IFSO driver action.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

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?

> 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?



================
Comment at: clang/lib/Driver/Driver.cpp:3449-3450
       return C.MakeAction<VerifyPCHJobAction>(Input, types::TY_Nothing);
+    if (Args.hasArg(options::OPT_emit_ifso))
+      return C.MakeAction<CompileJobAction>(Input, types::TY_IFO);
     return C.MakeAction<CompileJobAction>(Input, types::TY_LLVM_BC);
----------------
bikeshed: I don't think we should have both ifo and ifso. Either name sounds fine to me, but if running "-emit-ifso" gives me an ifo and not an ifso, that's very surprising, and is going to be a major headache trying to remember when to call it ifo and when to call it ifso.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60974