This is a continuation of D144596 , based on D144596#4182628.

Fix issue with gold plugin. Uses c++17 structured binding for getOldNewOptionsExtra for readability.

Adds missing %t to failing COFF test

Split thinLTOPrefixReplace into thinLTOPrefixReplaceOld and thinLTOPrefixReplaceNew because otherwise it was unexpeced that the first element of a pair would be replaced by a a variable that comes from outside the pair. Add test for COFF. Fixes typos and add comments to code.

All changes suggested in the comments have now been implemented

In D144596#4186838, @tejohnson wrote:

In D144596#4182584, @MaskRay wrote:

To support tree artifacts with an unknown number of full bitcode files and minimized bitcode files, we need to use different directories.
If these bitcode files are within the same directory, Bazel will have to ship both full and minimized bitcode files to a backend compile action, causing waste.
To ship just full bitcode files in a backend compile action, full and minimized bitcode files need to be in different directories (e.g. a.bc and indexing/a.bc).

I'm very confused as this is not mentioned anywhere in this patch by @itf. Normally, at least without tree artifacts, we only ship the specified input files, not the entire directory. The stated motivation for this particular patch, which was unrelated to the input files of the indexing action, was:

---

Bazel tree artifacts require that the folders are "write once". Therefore we cannot insert the link objects in the same folder as the result from the thinlto indexing step. Currently the file "--thinlto-index-only=file" requires the link objects to be in the same folder as the summary+import files from the indexing step.

This change allows having a different prefix for the link objects, and therefore keep the folders containing the summary+import files be "write once".

---

@itf can you clarify whether the input object handling is different than for non-tree artifacts as well?

replaced lld with %lld for MachO

In D144596#4151498, @MaskRay wrote:

In D144596#4151481, @itf wrote:

Can Bazel rewrite a.rsp with content xxx/a.o to xxx/random_hash/a.o? It will create a new file but I don't think that's a bottleneck.

Currently there is no code in bazel, that reads and modifies a parameter file like a.rsp. It is possible to do so by, for example, calling a shell script / binary executable that modifies the file and saves a new one as one of the actions (like so: https://github.com/bazelbuild/examples/tree/main/rules/actions_run).

However, I'd argue that it is better for lld to have this extra argument. The link objects .o are outputs of actions that can be run in parallel, while the thinlto.bc and the .import files are a result of the thinlto indexing action which run in series, i.e. all the thinlto.bc are generated simultaneously. I find it unexpected that lld would force those files to be in the same folders, given that thinlto-index-only= is particularly useful for distributed thinLTO, where there could be restrictions on what can be saved in each location

I am not convinced. For --thinlto-prefix-replace=';lto/;unique_prefix_for_rsp/', the output bitcode files and import files are in lto/ while the response file uses a different prefix.
This doesn't look like a reasonable build system request for the linker. Why can't Bazel place output bitcode files and import files under unique_prefix_for_rsp/ as well?

Can Bazel rewrite a.rsp with content xxx/a.o to xxx/random_hash/a.o? It will create a new file but I don't think that's a bottleneck.

Currently there is no code in bazel, that reads and modifies a parameter file like a.rsp. It is possible to do so by, for example, calling a shell script / binary executable that modifies the file and saves a new one as one of the actions (like so: https://github.com/bazelbuild/examples/tree/main/rules/actions_run).

IIUC the proposed third component of ; just changes the --thinlto-index-only=a.rsp specified response file (not anything else on the disk), then I am unsure how it is going to help Bazel...
Bazel can preprocess the a.rsp content itself, instead of requiring more from the linker.

Fixes build error caused by not including the new function getOldNewOptionsExtra in driver.cpp into the patch

In D144596#4146593, @MaskRay wrote:

What's the motivation of this change?