This is an archive of the discontinued LLVM Phabricator instance.

[ifs] Patch llvm-ifs to allow output multiple types of stub file at the same time
ClosedPublic

Authored by haowei on Dec 2 2021, 11:40 PM.

Details

Reviewers
phosek
mcgrathr
Summary

We are seeing use cases in outputting both ifs text stub and elf stub files from a single shared object in Fuchsia's build system. Instead of generating 2 llvm-ifs invocations, which will make llvm-ifs parse input ELF files twice, it is more efficient to just make llvm-ifs capable to generate every supported output format in a single run.

This patch adds "output-ifs", "output-elf" and "output-tbd" flags while still retain old "output-format", "output" flags just in case for compatibility. We can remove these old flags in a later revision.

Diff Detail

Event Timeline

haowei requested review of this revision.Dec 2 2021, 11:40 PM
haowei created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2021, 11:40 PM

This feature seems like a good addition. I'm wondering about the interactions with other switches and if it might make sense to rearchitect some of the CLI API here for the more arcane corners. Maybe it's already orthogonal enough, but I'm not sure. I'm thinking in particular about the various --strip-* options. Perhaps it's the case that all such variations apply only to text IFS output? I'm also wondering if there might be use cases for multiple different kinds of IFS output (i.e. different sets of stripping / arch options). Come to think of it, we could even have uses for multiple ELF stub outputs from the same IFS input with different switches (e.g. arch). So perhaps it makes sense to go in a direction where each output file is separately described with both its format (IFS vs ELF) and its format options (stripping, arch, etc), and there can be any number of such output files in whatever combination. I'm not sure exactly what a CLI syntax for that would look like. Another possibility that might fit well for build system integration is to accept a complex output specification in JSON either as a command-line switch value or from a file.

I suspect that even if we do want to pursue a fancier CLI API along those lines, this might be a reasonable incremental step in that direction.

haowei updated this revision to Diff 391779.Dec 3 2021, 4:41 PM
haowei retitled this revision from [ifs][WIP] Patch llvm-ifs to allow output multiple types of stub file at the same time to [ifs] Patch llvm-ifs to allow output multiple types of stub file at the same time.
haowei edited the summary of this revision. (Show Details)

I updated the unit tests and adds new tests for the new flags.

haowei added a comment.Dec 3 2021, 4:49 PM

This feature seems like a good addition. I'm wondering about the interactions with other switches and if it might make sense to rearchitect some of the CLI API here for the more arcane corners. Maybe it's already orthogonal enough, but I'm not sure. I'm thinking in particular about the various --strip-* options. Perhaps it's the case that all such variations apply only to text IFS output? I'm also wondering if there might be use cases for multiple different kinds of IFS output (i.e. different sets of stripping / arch options). Come to think of it, we could even have uses for multiple ELF stub outputs from the same IFS input with different switches (e.g. arch). So perhaps it makes sense to go in a direction where each output file is separately described with both its format (IFS vs ELF) and its format options (stripping, arch, etc), and there can be any number of such output files in whatever combination. I'm not sure exactly what a CLI syntax for that would look like. Another possibility that might fit well for build system integration is to accept a complex output specification in JSON either as a command-line switch value or from a file.

I suspect that even if we do want to pursue a fancier CLI API along those lines, this might be a reasonable incremental step in that direction.

The --strip-* options only work when output is (has) IFS and have no effect on other type of outputs. The helper text has indicate these flags works on IFS output (only). I think this was specified in the original design doc as well.

I agree it might be an opportunity to support multiple output, not just multiple types of output. My first thought is something like:

--output-elf='elfA.so,--target=x86_64-linux-gnu' --output-elf='elfB.so,--target=aarch64-fuchsia-unknown

Essentially, we want some kind of tuple for each output file so we can specify information like targets. But that also mean we need an overhaul of flags parsing as current cl::opt does not seem to support it.

This feature seems like a good addition. I'm wondering about the interactions with other switches and if it might make sense to rearchitect some of the CLI API here for the more arcane corners. Maybe it's already orthogonal enough, but I'm not sure. I'm thinking in particular about the various --strip-* options. Perhaps it's the case that all such variations apply only to text IFS output? I'm also wondering if there might be use cases for multiple different kinds of IFS output (i.e. different sets of stripping / arch options). Come to think of it, we could even have uses for multiple ELF stub outputs from the same IFS input with different switches (e.g. arch). So perhaps it makes sense to go in a direction where each output file is separately described with both its format (IFS vs ELF) and its format options (stripping, arch, etc), and there can be any number of such output files in whatever combination. I'm not sure exactly what a CLI syntax for that would look like. Another possibility that might fit well for build system integration is to accept a complex output specification in JSON either as a command-line switch value or from a file.

I suspect that even if we do want to pursue a fancier CLI API along those lines, this might be a reasonable incremental step in that direction.

The --strip-* options only work when output is (has) IFS and have no effect on other type of outputs. The helper text has indicate these flags works on IFS output (only). I think this was specified in the original design doc as well.

I agree it might be an opportunity to support multiple output, not just multiple types of output. My first thought is something like:

--output-elf='elfA.so,--target=x86_64-linux-gnu' --output-elf='elfB.so,--target=aarch64-fuchsia-unknown

Essentially, we want some kind of tuple for each output file so we can specify information like targets. But that also mean we need an overhaul of flags parsing as current cl::opt does not seem to support it.

Alternative would be to make --target= position significant; every time it's set, it affects the flags that follow. So your example would look like this:

--target=x86_64-linux-gnu --output-elf=elfA.so --target=aarch64-fuchsia-unknown --output-elf=elfB.so

That matches some of the other tools like ld which have position significant flags like -Bdynamic and -Bstatic.

cl::opt wouldn't handle this, but I think llvm-ifs should switch to OptTable anyway, and it should be implementable with OptTable similarly to how lld does it.

haowei updated this revision to Diff 392957.Dec 8 2021, 3:31 PM

Fix some "no newline" warnings.

phosek accepted this revision.Dec 9 2021, 10:19 AM

LGTM

llvm/tools/llvm-ifs/llvm-ifs.cpp
496

Does this mean that if someone passes --output-elf=foo --output-elf=bar we won't take this branch because OutputELFFilePath.getNumOccurrences() == 2?

This revision is now accepted and ready to land.Dec 9 2021, 10:19 AM
haowei added inline comments.Dec 9 2021, 10:33 AM
llvm/tools/llvm-ifs/llvm-ifs.cpp
496

Ah, you got a point. Yes, it won't take this branch. I should add error that these flags cannot be used more than once, at least in this revision.

phosek added inline comments.Dec 9 2021, 10:36 AM
llvm/tools/llvm-ifs/llvm-ifs.cpp
496

Another option would be to allow these flags to be set multiple times and follow the "last one wins" strategy.

haowei added inline comments.Dec 9 2021, 11:33 AM
llvm/tools/llvm-ifs/llvm-ifs.cpp
496
./llvm-ifs --input-format=ELF --strip-ifs-target --output-ifs=./testA.ifs --output-ifs=./testB.ifs ./llvm-ifs
llvm-ifs: for the --output-ifs option: may only occur zero or one times!

Looks like by default, 'cl::opt' only support zero or 1 time so this is not an issue.

haowei closed this revision.Jan 7 2022, 10:27 AM