| Summary: | dlltool broke in 2.38 | ||
|---|---|---|---|
| Product: | binutils | Reporter: | Mikael Pettersson <mikpelinux> |
| Component: | binutils | Assignee: | Not yet assigned to anyone <unassigned> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | bshanks, martin |
| Priority: | P2 | ||
| Version: | 2.38 | ||
| Target Milestone: | --- | ||
| Host: | Target: | ||
| Build: | Last reconfirmed: | ||
| Attachments: | Potential fix | ||
|
Description
Mikael Pettersson
2022-02-12 15:27:37 UTC
I think the problem is that multiple instances of dlltool may use the same temporary file names depending on the files being built.
With fdeee5d59dca41e3c70c399a939105e39a4b4282, the temporary file prefix will be taken from the .def file, and the .def files may not have unique names. For example, in mingw-w64-crt/lib64, d3dcompiler_{33,34,35,36}.def all have the same LIBRARY name inside: "D3DCompiler.dll" [1].
I've also seen this bug when building Wine (on both Linux and macOS), where multiple dlltools may be launched at the same time to build delay-load and cross libraries for the same DLL (using the same .spec as input).
I'm not sure of the best fix for this: maybe add a command-line option to use a deterministic tmp_prefix, with the caveat that parallel builds may fail?
(As an aside, I'm not sure why one would want deterministic tmp file names. Maybe for reproducible builds?)
[1]: https://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/mingw-w64-crt/lib64/d3dcompiler_33.def
(In reply to Brendan Shanks from comment #1) > With fdeee5d59dca41e3c70c399a939105e39a4b4282, the temporary file prefix > will be taken from the .def file, and the .def files may not have unique > names. For example, in mingw-w64-crt/lib64, d3dcompiler_{33,34,35,36}.def > all have the same LIBRARY name inside: "D3DCompiler.dll" [1]. Sorry about this - I really didn't forsee that this would be an issue. > I've also seen this bug when building Wine (on both Linux and macOS), where > multiple dlltools may be launched at the same time to build delay-load and > cross libraries for the same DLL (using the same .spec as input). In wine, what cases of multiple parallel import libraries targeting the same DLL name? > I'm not sure of the best fix for this: maybe add a command-line option to > use a deterministic tmp_prefix, with the caveat that parallel builds may > fail? That doesn't sound very ideal to me. Ideally it would do the right thing, deterministically, out of the box, without conflict. If the issue is that the deterministic temp file names can clash when running multiple dlltool invocations in parallel, should we perhaps create a separate temp directory (where the name can be nondeterministic), where we make sure we get a unique directory that nobody else is using, then write all temp files within that one? Or should we make the deterministic temp prefix based on something else, e.g. the ouput (path+)filename? That should be unique even with multiple jobs running in parallel, if they're running in the same directory. The temp prefix base might end up slightly longer, so it's maybe a little less elegant, but should be much safer against clashes. > (As an aside, I'm not sure why one would want deterministic tmp file names. > Maybe for reproducible builds?) Msys2 had issues with the nondeterministic nature of dlltool. In most cases, the temp prefix doesn't matter - but for umbrella libraries like libucrt.a, which is merged from a number of individual import libraries like this, https://github.com/mingw-w64/mingw-w64/blob/master/mingw-w64-crt/lib-common/ucrt.mri, it matters: The temp prefix for all the individual libraries making up libucrt.a must be unique, otherwise ld.bfd mixes up the object files for those libraries, producing intermixed import tables for those DLLs. See https://github.com/msys2/MINGW-packages/issues/9363#issuecomment-899100856 for the root cause. We worked around this with https://github.com/mingw-w64/mingw-w64/commit/0ad299826ca14987fd53664c1047f4dfe848c3f8, which adds the --temp-prefix option (based on the output import library name, which should be unique) if the option is supported. We also noticed that Debian had been using the --temp-prefix option in this way to make their builds reproducible: https://salsa.debian.org/mingw-w64-team/mingw-w64/-/blob/master/debian/patches/dlltool-temp-prefix.patch Based on this, I thought it'd be good if we could make dlltool fix both of these issues out of the box, so that users of it don't need to take extra steps to both achieve reproducibility and avoid the intermixed import tables. (Also, if building a newer version of mingw-w64 that contains the commit above, which explicitly sets the --temp-prefx option, I think this bug wouldn't be noticed at all. So, good that you've caught it at least!) Created attachment 14013 [details]
Potential fix
Here's a pretty straightforward patch that uses the output filename as basis for the temp prefix, which I think should avoid this issue (if there's no other aspect I'm overlooking). Does this fix the issue for you?
(In reply to Martin Storsjö from comment #3) > Created attachment 14013 [details] > Potential fix > > Here's a pretty straightforward patch that uses the output filename as basis > for the temp prefix, which I think should avoid this issue (if there's no > other aspect I'm overlooking). Does this fix the issue for you? Thanks Martin, that worked for me for a Wine build. Mikael, can you try building mingw-w64's crt? (In reply to Brendan Shanks from comment #4) > (In reply to Martin Storsjö from comment #3) > > Created attachment 14013 [details] > > Potential fix > > > > Here's a pretty straightforward patch that uses the output filename as basis > > for the temp prefix, which I think should avoid this issue (if there's no > > other aspect I'm overlooking). Does this fix the issue for you? > > Thanks Martin, that worked for me for a Wine build. Mikael, can you try > building mingw-w64's crt? I managed to reproduce the issue building mingw-w64-crt, and the patch does fix that case too. (I could also verify that building the latest git master version of mingw-w64-crt isn't affected by the bug, as those versions explicitly specify unique --temp-prefix strings.) I'll go ahead and send the patch for review then. (In reply to Brendan Shanks from comment #4) > (In reply to Martin Storsjö from comment #3) > > Created attachment 14013 [details] > > Potential fix > > > > Here's a pretty straightforward patch that uses the output filename as basis > > for the temp prefix, which I think should avoid this issue (if there's no > > other aspect I'm overlooking). Does this fix the issue for you? > > Thanks Martin, that worked for me for a Wine build. Mikael, can you try > building mingw-w64's crt? After applying the proposed fix on top of binutils-2.38 I can no longer reproduce the problem. The master branch has been updated by Nick Clifton <nickc@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=d65c0ddddd85645cab6f11fd711d21638a74489f commit d65c0ddddd85645cab6f11fd711d21638a74489f Author: Martin Storsj? <martin@martin.st> Date: Wed Mar 16 15:01:30 2022 +0000 dlltool: Use the output name as basis for deterministic temp prefixes PR 28885 * dlltool.c (main): use imp_name rather than dll_name when generating a temporary file name. Fixed on master for 2.39, needs backport to binutils-2_38-branch. The binutils-2_38-branch branch has been updated by Nick Clifton <nickc@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=99852365513266afdd793289813e8e565186c9e6 commit 99852365513266afdd793289813e8e565186c9e6 Author: Nick Clifton <nickc@redhat.com> Date: Wed Mar 23 11:39:49 2022 +0000 dlltool: Use the output name as basis for deterministic temp prefixes PR 28885 * dlltool.c (main): use imp_name rather than dll_name when generating a temporary file name. And now fixed on binutils-2_38-branch, closing. |