[llvm-dev] [ThinLTO] static library failure with object files with the same name

[Johan Engelen via llvm-dev]

Hi all,

  I have a static library with object files with the same name (not the same full path, but the archive made with llvm-ar does not store the full path). The library contains object files that have been compiled with `-flto=thin` (compiled with LDC, not clang, but that shouldn't matter).

When linking to that static library, I get the error:

Assertion failed: (ModuleMap.find(ModuleBuffer.getBufferIdentifier()) == ModuleMap.end() && "Expect unique Buffer Identifier"), function generateModuleMap, file ../lib/LTO/ThinLTOCodeGenerator.cpp, line 138.

The error occurs because the buffer identifier uses the filename of the objects inside the archive, and those are identical for the two files with different source path.

This problem appears to be fixed for LLD here:

https://reviews.llvm.org/D25495

https://bugs.llvm.org/show_bug.cgi?id=30665

But it persists for linking with the system linker on OSX (while manually passing libLTO.dylib to the linker).

If I modify lib/LTO/ThinLTOCodeGenerator.cpp to do a poor-man's uniquefying of the buffer identifier:

```

void ThinLTOCodeGenerator::addModule(StringRef Identifier, StringRef Data) {

-  ThinLTOBuffer Buffer(Data, Identifier.str()));

+  ThinLTOBuffer Buffer(Data, Identifier.str() + utostr(Data.size()));

```

then the problem is solved.

What would be a proper way to fix this issue?

Can it be fixed in lib/LTO, or should I not create an (afaict valid) archive containing duplicate object file names?

(Note: this issue makes it impossible to use an LTO version of the standard library with LDC)

Thanks,

  Johan

[Teresa Johnson via llvm-dev]

Hi Johan,

Right, per the bug this is fixed in lld (and was already handled in gold-plugin), but I guess not in ld64. Note that lld and gold-plugin use the new LTO API, while ld64 (and probably other linkers) are still using the legacy libLTO (which is what ThinLTOCodeGenerator.cpp is part of). Fixing it in the location you propose could work for all legacy libLTO users. But I don't think that adding just the size will (always) be enough to disambiguate (couldn't the 2 same named members have the same size?) - although lld is doing the same thing so this may be as good as what is done there. For gold-plugin we add the byte offset into the archive where the member starts, which will be unique.

+davide for thoughts since he fixed it on the lld side.

Teresa

_______________________________________________
LLVM Developers mailing list
llvm...@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

--

Teresa Johnson |

Software Engineer |

tejo...@google.com |

408-460-2413

[Davide Italiano via llvm-dev]

On Tue, Sep 5, 2017 at 2:09 PM, Teresa Johnson <tejo...@google.com> wrote:
>
> Hi Johan,
>
> Right, per the bug this is fixed in lld (and was already handled in gold-plugin), but I guess not in ld64. Note that lld and gold-plugin use the new LTO API, while ld64 (and probably other linkers) are still using the legacy libLTO (which is what ThinLTOCodeGenerator.cpp is part of). Fixing it in the location you propose could work for all legacy libLTO users. But I don't think that adding just the size will (always) be enough to disambiguate (couldn't the 2 same named members have the same size?) - although lld is doing the same thing so this may be as good as what is done there. For gold-plugin we add the byte offset into the archive where the member starts, which will be unique.
> +davide for thoughts since he fixed it on the lld side.
>

Yes, Teresa is right, this is the correct fix.
I'm not sure what subset of GNU archives Mach-O supports, but the only
way of being safe is using offset in the archive + archive name.
Fun fact, you apparently can have a single GNU archive with two
different members with the same name. Using the offset is the only way
to disambiguate.
I think we should really consider documenting this assumption
somewhere as many people have been bitten in the past and tracking
down is not trivial as it shows up as assertion failures in the mover
or duplicate/undefined symbols reported as linker errors, as the
ThinLTO logic will pick the an archive randomly.

[Johan Engelen via llvm-dev]

On Tue, Sep 5, 2017 at 11:34 PM, Davide Italiano <dccit...@gmail.com> wrote:

On Tue, Sep 5, 2017 at 2:09 PM, Teresa Johnson <tejo...@google.com> wrote:
>
> Hi Johan,
>
> Right, per the bug this is fixed in lld (and was already handled in gold-plugin), but I guess not in ld64. Note that lld and gold-plugin use the new LTO API, while ld64 (and probably other linkers) are still using the legacy libLTO (which is what ThinLTOCodeGenerator.cpp is part of). Fixing it in the location you propose could work for all legacy libLTO users. But I don't think that adding just the size will (always) be enough to disambiguate (couldn't the 2 same named members have the same size?) - although lld is doing the same thing so this may be as good as what is done there. For gold-plugin we add the byte offset into the archive where the member starts, which will be unique.
> +davide for thoughts since he fixed it on the lld side.
>

Yes, Teresa is right, this is the correct fix.
I'm not sure what subset of GNU archives Mach-O supports, but the only
way of being safe is using offset in the archive + archive name.

ThinLTOCodeGenerator::addModule is called by the linker, right? (I can't find any callers)

When it is called, we cannot retrieve the offset of the module inside the archive, because the linker didn't tell us about it.

What we could do is retrieve the module hash from the buffer (although perhaps it is not always there with ThinLTO?), and use that for disambiguation?

I am assuming that we do want to assert/error on calling addModule with the exact same module twice? Otherwise, doing a search for the identifier in Modules vector first and disambiguate if found would work (multithreading would need to be taken into account too I think?).

- Johan

[Davide Italiano via llvm-dev]

See here for the fix.
https://reviews.llvm.org/D25495
You pass the the archive name + offset to `lto::InputFile::create`,
assuming your linker uses the new LTO API (and I'm not sure whether
ld64 already switched).
The linker knows the archive name from which it's fetching the member,
as well as its offset (it asks libArchive about it, for lld).
I'm not sure how it works ld64, but of course, to get this mechanism
working, you need some linker modifications.
CC:ed some Mach-O people, they probably know more about this than I do.

Thanks,

--
Davide

[Johan Engelen via llvm-dev]

My first priority is fixing the old API so that we can release LDC with ThinLTO working for the stdlib on current OSX setups. I'll probably stick with adding the size to the identifier for simplicity (we have our own fork of LLVM for releasing).

From what I can tell from online sources, latest ld64 (274.2) is using the old api.

 

CC:ed some Mach-O people, they probably know more about this than I do.

Thanks.

- Johan

 

[Teresa Johnson via llvm-dev]

AFAIK, no. Mehdi started to look at this awhile back but likely has not been able to pick it back up.

 

The linker knows the archive name from which it's fetching the member,
as well as its offset (it asks libArchive about it, for lld).
I'm not sure how it works ld64, but of course, to get this mechanism
working, you need some linker modifications.

Right, the linker can compose the new identifier. The proposed fix in ThinLTOCodeGenerator.cpp could be a partial workaround until linkers do this.

Agree that it should be documented, at the very least in the header file interfaces to the new/old LTO APIs. Not sure there is a better place to document?

Teresa

 

CC:ed some Mach-O people, they probably know more about this than I do.

Thanks,

--
Davide

_______________________________________________
LLVM Developers mailing list
llvm...@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

[Mehdi AMINI via llvm-dev]

Hi Johan,

ld64 only calls functions from llvm/include/llvm-c/lto.h  (defined in llvm/tools/lto/lto.cpp)

For instance ThinLTOCodeGenerator::addModule is called through thinlto_codegen_add_module().

Apple hasn't released the code for ld64 in Xcode 9 yet, did you check if it is fixed in Xcode 9?

(I think I remember fixing it in ld64 but I'm not totally sure...).

From what I can see with Xcode 8.2, the linker just passes the file name (prefixed with the archive name): https://github.com/michaelweiser/ld64/blob/master/src/ld/parsers/lto_file.cpp#L546

(original here: https://opensource.apple.com/source/ld64/ld64-274.2/src/ld/parsers/lto_file.cpp.auto.html )

We could workaround this in ThinLTOCodeGenerator by adding a incremental suffix to every new buffer. Something like this diff:

diff --git a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp

index ffd78dad9228..d6e5d4d0c213 100644

--- a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp

+++ b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp

@@ -535,7 +535,9 @@ static void initTMBuilder(TargetMachineBuilder &TMBuilder,

 } // end anonymous namespace

 

 void ThinLTOCodeGenerator::addModule(StringRef Identifier, StringRef Data) {

-  ThinLTOBuffer Buffer(Data, Identifier);

+  std::string Id =

+      (Twine(Identifier) + "_" + std::to_string(Modules.size())).str();

+  ThinLTOBuffer Buffer(Data, std::move(Id));

   LLVMContext Context;

   StringRef TripleStr;

   ErrorOr<std::string> TripleOrErr = expectedToErrorOrAndEmitErrors(

-- 

Mehdi

[Johan Engelen via llvm-dev]

On Thu, Sep 7, 2017 at 5:44 PM, Mehdi AMINI <joke...@gmail.com> wrote:

Hi Johan,

ld64 only calls functions from llvm/include/llvm-c/lto.h  (defined in llvm/tools/lto/lto.cpp)

For instance ThinLTOCodeGenerator::addModule is called through thinlto_codegen_add_module().

Apple hasn't released the code for ld64 in Xcode 9 yet, did you check if it is fixed in Xcode 9?

(I think I remember fixing it in ld64 but I'm not totally sure...).

I haven't tried with Xcode 9.

 

From what I can see with Xcode 8.2, the linker just passes the file name (prefixed with the archive name): https://github.com/michaelweiser/ld64/blob/master/src/ld/parsers/lto_file.cpp#L546

(original here: https://opensource.apple.com/source/ld64/ld64-274.2/src/ld/parsers/lto_file.cpp.auto.html )

We could workaround this in ThinLTOCodeGenerator by adding a incremental suffix to every new buffer. Something like this diff:

I was assuming that we do want to assert/error on calling addModule with the exact same module twice? Otherwise your diff is nice, thanks.

 

- Johan

[Johan Engelen via llvm-dev]

On Fri, Sep 8, 2017 at 9:04 PM, Johan Engelen <jbc.e...@gmail.com> wrote:

On Thu, Sep 7, 2017 at 5:44 PM, Mehdi AMINI <joke...@gmail.com> wrote:

Hi Johan,

ld64 only calls functions from llvm/include/llvm-c/lto.h  (defined in llvm/tools/lto/lto.cpp)

For instance ThinLTOCodeGenerator::addModule is called through thinlto_codegen_add_module().

Apple hasn't released the code for ld64 in Xcode 9 yet, did you check if it is fixed in Xcode 9?

(I think I remember fixing it in ld64 but I'm not totally sure...).

I haven't tried with Xcode 9.

 

From what I can see with Xcode 8.2, the linker just passes the file name (prefixed with the archive name): https://github.com/michaelweiser/ld64/blob/master/src/ld/parsers/lto_file.cpp#L546

(original here: https://opensource.apple.com/source/ld64/ld64-274.2/src/ld/parsers/lto_file.cpp.auto.html )

We could workaround this in ThinLTOCodeGenerator by adding a incremental suffix to every new buffer. Something like this diff:

I was assuming that we do want to assert/error on calling addModule with the exact same module twice? Otherwise your diff is nice, thanks.

 

Hi Mehdi,

  Can you advise me?

Is it OK to not error upon the exact same module being added twice? (and thus your patch would be good)

Thanks,

[Mehdi AMINI via llvm-dev]

Hi Johan,

2017-09-11 14:21 GMT-07:00 Johan Engelen <jbc.e...@gmail.com>:

On Fri, Sep 8, 2017 at 9:04 PM, Johan Engelen <jbc.e...@gmail.com> wrote:

On Thu, Sep 7, 2017 at 5:44 PM, Mehdi AMINI <joke...@gmail.com> wrote:

Hi Johan,

ld64 only calls functions from llvm/include/llvm-c/lto.h  (defined in llvm/tools/lto/lto.cpp)

For instance ThinLTOCodeGenerator::addModule is called through thinlto_codegen_add_module().

Apple hasn't released the code for ld64 in Xcode 9 yet, did you check if it is fixed in Xcode 9?

(I think I remember fixing it in ld64 but I'm not totally sure...).

I haven't tried with Xcode 9.

 

From what I can see with Xcode 8.2, the linker just passes the file name (prefixed with the archive name): https://github.com/michaelweiser/ld64/blob/master/src/ld/parsers/lto_file.cpp#L546

(original here: https://opensource.apple.com/source/ld64/ld64-274.2/src/ld/parsers/lto_file.cpp.auto.html )

We could workaround this in ThinLTOCodeGenerator by adding a incremental suffix to every new buffer. Something like this diff:

I was assuming that we do want to assert/error on calling addModule with the exact same module twice? Otherwise your diff is nice, thanks.

 

Hi Mehdi,

  Can you advise me?

Is it OK to not error upon the exact same module being added twice? (and thus your patch would be good)

It is a matter of API contract, so I'm not sure I perceive a clear right/wrong here. If we want to enforce an API contract on the caller's responsibility to provide a unique ID, then the assert may be OK (however we disable assertions in production and it means random crashes which isn't super user-friendly).

On the other hand my patch here was trying to be resilient to API misuse for "little cost".

Note that the plan was that this API (and the ThinLTOCodegenerator.cpp) should just die, but unfortunately I haven't had time to work on the replacement (a C binding for the new LTO API).

-- 

Mehdi

[Johan Engelen via llvm-dev]

I've created a review for your patch Mehdi: https://reviews.llvm.org/D37961

First time using `arc`, so hope things went well.

- Johan

[Johan Engelen via llvm-dev]

The fix (https://reviews.llvm.org/D37961) does not work.  From what I have learned thusfar, the module identifier is used as filename sometimes (I think when writing an intermediate module index summary), and so a bunch of lit tests fail with the "fix".

I'll look further into fixing this, any help is appreciated.

( One thing that may be important is to have a deterministic suffix. The counter solution of https://reviews.llvm.org/D37961 would name the same module differently depending on order, which may happen when doing thinlto in several different steps. Adding the size of the module (or its offset) would be the same in each invocation. Regardless, changing the suffix to a size suffix does not fix the lit test problems. )

regards,

  Johan

[Mehdi AMINI via llvm-dev]

It is expected and not unusual to need to update the lit test in such case. I'd need to see exactly which test breaks and how to know though.

Best,

-- 

Mehdi

[Johan Engelen via llvm-dev]

This is a typical error:

http://green.lab.llvm.org/green/job/clang-stage1-configure-RA_check/35542/testReport/junit/LLVM/ThinLTO_X86/funcimport_tbaa_ll/

"

llvm-lto: error loading file '/Users/buildslave/jenkins/sharedspace/phase1@2/clang-build/test/ThinLTO/X86/Output/funcimport-tbaa.ll.tmp.bc_0': No such file or directory

"

Note the "_0" suffix. I think what happens is that the suffixed module identifier is used as filename when writing out the thinlto index, and in a subsequent step that file can (of course) not be found.

Other lit errors have to do with symbols not being found correctly across modules, leading e.g. to wrong internalization/optimizations after importing.

List of failures: http://green.lab.llvm.org/green/job/clang-stage1-configure-RA_check/35542/testReport/junit/LLVM/

regards,

  Johan

[Johan Engelen via llvm-dev]

Hi all,

  Just a quick note to say that the problem is gone after updating to XCode 9. As Mehdi remembered, it's fixed in the new ld64 (thanks!).

Cheers,

  Johan