[PATCH] D66767: Add binary filename to the bitcode filename when using -thinlto-index-only

Mon Aug 26 16:18:59 PDT 2019

inglorion added a reviewer: ruiu.
inglorion added a subscriber: ruiu.
inglorion added a comment.

@ruiu, I'd like your opinion on this.

TLDR; I lean towards passing on this change because we already have -thinlto-prefix-replace, but automatically coming up with output-dependent names for files that need to be different depending on output is nice so perhaps others think differently. My thoughts:

This includes the (base)name of the output in the name of the bitcode file buffer when using -thinlto-index-only. That name ends up being used in diagnostics and for distributed ThinLTO files (.thinlto.bc) and imports files, as well as the name we name we record for the file the symbols came from. So a file foo.obj in libfoobar.lib being linked into test.exe used to be called "libfoobar.libfoo.obj576" and would now be called "libfoobar.libfoo.obj576.test.exe" when -thinlto-index-only is in use, and the ThinLTO index would be "libfoobar.libfoo.obj576.test.exe.thinlto.bc" instead of "libfoobar.libfoo.obj576.thinlto.bc".

Without this change, we can achieve unique names for index and imports files by using the -thinlto-prefix-replace flag, e.g. "-thinlto-prefix-replace=;test.exe." would replace the empty prefix with "test.exe.", so that "libfoobar.libfoo.obj576" would become "test.exe.libfoobar.libfoo.obj576". Aside from the different naming scheme, the main difference is that this change makes the name uniquing automatic. I think there is a case to be made for that, but I'm not 100% sold. One objection is that the new names are not guaranteed to be unique; for example, two outputs called "test.exe" in different directories will generate the same names. With -thinlto-prefix-replace, because the renaming needs to be explicitly specified, we punt responsibility for unique names to the user, rather than doing something that will usually work but sometimes lead to incorrect results. Given that I'm not 100% sold and we have an existing mechanism, I lean towards not taking the change.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66767