[Bug binutils/12633] Crash with dlltool geneated import *.libs in MSVC++ Release target (Debug OK)
http://sourceware.org/bugzilla/show_bug.cgi?id=12633 Martin Storsjö changed: What|Removed |Added CC||martin at martin dot st --- Comment #3 from Martin Storsjö 2013-05-0 9 17:12:19 UTC --- For what it's worth, it seems that this issue has been fixed in the MSVC 20 12 linker - there binaries linked with /OPT:REF against dlltool-produced import libraries seem to work just fine. -- Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ bug-binutils mailing list bug-binutils@gnu.org https://lists.gnu.org/mailman/listinfo/bug-binutils
[Bug binutils/12633] Crash with dlltool geneated import *.libs in MSVC++ Release target (Debug OK)
http://sourceware.org/bugzilla/show_bug.cgi?id=12633 --- Comment #4 from Martin Storsjö --- This issue seems to go away with the latest version of the linker from MSVC 2012 and 2013, so I guess it can be considered a bug in their side. -- You are receiving this mail because: You are on the CC list for the bug. ___ bug-binutils mailing list bug-binutils@gnu.org https://lists.gnu.org/mailman/listinfo/bug-binutils
[Bug binutils/28885] dlltool broke in 2.38
https://sourceware.org/bugzilla/show_bug.cgi?id=28885
--- Comment #2 from Martin Storsjö ---
(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!)
--
You are receiving this mail because:
You are on the CC list for the bug.
[Bug binutils/28878] [RISCV] Incorrect DW_AT_high_pc for assembly source with linker relaxation
https://sourceware.org/bugzilla/show_bug.cgi?id=28878 Martin Storsjö changed: What|Removed |Added CC|martin at martin dot st| -- You are receiving this mail because: You are on the CC list for the bug.
[Bug binutils/28885] dlltool broke in 2.38
https://sourceware.org/bugzilla/show_bug.cgi?id=28885 --- Comment #3 from Martin Storsjö --- Created attachment 14013 --> https://sourceware.org/bugzilla/attachment.cgi?id=14013&action=edit 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? -- You are receiving this mail because: You are on the CC list for the bug.
[Bug binutils/28885] dlltool broke in 2.38
https://sourceware.org/bugzilla/show_bug.cgi?id=28885 --- Comment #5 from Martin Storsjö --- (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. -- You are receiving this mail because: You are on the CC list for the bug.
[Bug ld/30140] LD does not link PE executable with symbols from DEF file
https://sourceware.org/bugzilla/show_bug.cgi?id=30140 Martin Storsjö changed: What|Removed |Added CC||martin at martin dot st --- Comment #2 from Martin Storsjö --- > I think that this is an issue in LD because it should be able to handle DEF > file > correctly. Or if LD does not support DEF files then it should throw > meaningful human > readable error. Not that "symbol not defined" because it *is* defined in > test-library.def file. The issue is that LD _does_ take DEF files as input - but not in the way you seem to assume. When you link a DLL (or EXE for that matter) and pass it a DEF file at the same time, the DEF file overrides what symbols to export, instead of the regular logic (functions marked dllexport, or exporting everything modulo explicit ignores, etc). See the section in the manual: https://sourceware.org/binutils/docs/ld/WIN32.html#index-using-a-DEF-file -- You are receiving this mail because: You are on the CC list for the bug.
[Bug binutils/23348] binutils strip corrupts binaries produced by clang+lld+mingw-w64 toolchain
https://sourceware.org/bugzilla/show_bug.cgi?id=23348 Martin Storsjö changed: What|Removed |Added CC||martin at martin dot st -- You are receiving this mail because: You are on the CC list for the bug. ___ bug-binutils mailing list bug-binutils@gnu.org https://lists.gnu.org/mailman/listinfo/bug-binutils
[Bug ld/22762] missing static variable constructor calls
https://sourceware.org/bugzilla/show_bug.cgi?id=22762
Martin Storsjö changed:
What|Removed |Added
CC||martin at martin dot st
--- Comment #15 from Martin Storsjö ---
(In reply to Hannes Domani from comment #12)
> (In reply to Nick Clifton from comment #9)
> > Well I have been wracking my brains for a couple of days now, and I cannot
> > think of a better solution either.
> You first suggested this:
> > This would imply that the placement of the __CTOR_LIST__ symbol in
> > libgcc(_ctors.o) is incorrect,
> So is the definition in libgcc2.c even necessary, isn't it always provided
> by the linker?
It's not normally needed, no. As far as I understand it, I think libgcc just
provides the symbol in case nothing else provides it (maybe for other targets
than mingw).
> On the other hand, I don't know why anyone would want to override
> __CTOR_LIST__ anyways.
This issue originated from trying to use mingw-w64 with lld, llvm's linker
(which basically works like msvc's link.exe with a few additions), and doesn't
provide __CTOR_LIST__ and doesn't use linker scripts at all.
When intending to use that linker, we provide __CTOR_LIST__ within mingw-w64's
crt startup object files. If binutils ld would be able to handle this case
(which the original patch achieves), it would (at a later stage when one could
start requiring binutils >= 2.30) reduce the amount of conditionals and the
fact that we need to know what linker we're going to use, when building
mingw-w64.
If we'd enable this codepath for all cases, we'd fix this issue for binutils
2.30 (but at the same time break all earlier versions).
Other potential ways of handling it would be to extend libgcc's fallback
__CTOR_LIST__ to something like this:
__attribute__ (( __section__ (".ctors"), __used__ , aligned(sizeof(void *
const void * __CTOR_LIST__ = (void *) -1;
__attribute__ (( __section__ (".dtors"), __used__ , aligned(sizeof(void *
const void * __DTOR_LIST__ = (void *) -1;
__attribute__ (( __section__ (".ctors.9"), __used__ , aligned(sizeof(void
* const void * __CTOR_END__ = (void *) 0;
__attribute__ (( __section__ (".dtors.9"), __used__ , aligned(sizeof(void
* const void * __DTOR_END__ = (void *) 0;
However I think that only works if the object file that provides it (in the
case of mingw-w64 for lld, in crt2.o) is linked first, before everything else,
placing __CTOR_LIST__ at the head of the .ctors section. And I don't think
that's fit for libgcc since I guess it's too target specific, and would still
cause breakage for any current libgcc version that doesn't have it.
So given that, I guess reverting the patch is the most sensible thing to do,
and we'll have to start over with other approaches, if we want to unify
matters.
--
You are receiving this mail because:
You are on the CC list for the bug.
___
bug-binutils mailing list
bug-binutils@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-binutils
[Bug binutils/23348] binutils strip corrupts binaries produced by clang+lld+mingw-w64 toolchain
https://sourceware.org/bugzilla/show_bug.cgi?id=23348 --- Comment #2 from Martin Storsjö --- This doesn't seem to be an issue with binutils, but with LLD; initial fix suggestions for LLD are at https://reviews.llvm.org/D49350 and https://reviews.llvm.org/D49351. -- You are receiving this mail because: You are on the CC list for the bug. ___ bug-binutils mailing list bug-binutils@gnu.org https://lists.gnu.org/mailman/listinfo/bug-binutils
[Bug binutils/23348] binutils strip corrupts binaries produced by clang+lld+mingw-w64 toolchain
https://sourceware.org/bugzilla/show_bug.cgi?id=23348 --- Comment #3 from Martin Storsjö --- This should have been fixed now in LLD SVN r337598. -- You are receiving this mail because: You are on the CC list for the bug. ___ bug-binutils mailing list bug-binutils@gnu.org https://lists.gnu.org/mailman/listinfo/bug-binutils
[Bug ld/23762] __CTOR_LIST__ should be aligned at 4 bytes for i686-windows-mingw32
https://sourceware.org/bugzilla/show_bug.cgi?id=23762 --- Comment #1 from Martin Storsjö --- While the conclusion probably is right, I'd like to see more details about why this happens in this configuration and why it hasn't happened so far. As far as I can see, ld places __CTOR_LIST__ directly at the end of the .text section - and in case the .text section isn't exactly a multiple of 4, the -1 at the beginning would be unaligned, and there would be garbage/fillers between that and the first member from .ctors (which is aligned) - is this what is happening? Then why does this not happen in normal setups with GCC, since GCC also does align the .ctors pointers to 4 bytes? -- You are receiving this mail because: You are on the CC list for the bug. ___ bug-binutils mailing list bug-binutils@gnu.org https://lists.gnu.org/mailman/listinfo/bug-binutils
[Bug ld/23872] MinGW Binaries can be built with misaligned relocation information
https://sourceware.org/bugzilla/show_bug.cgi?id=23872 Martin Storsjö changed: What|Removed |Added CC||martin at martin dot st --- Comment #3 from Martin Storsjö --- (In reply to Nick Clifton from comment #2) > > It sounds a bit suspicious that clang is not aligning the relocs Just for discussion, clang doesn't align the relocs (as they are produced by ld) - I presume you meant why clang doesn't align the rdata section. Judging from the linker script, wouldn't it mostly be a case of clang producing object files with .rdata sections that end with an uneven number of bytes? And I don't see how that would be suspicious. I haven't dug into a full case of this happening though. In any case, the applied patch certainly is right though. (I'm just arguing because I don't think there is a bug to be fixed on the clang side wrt this, as someone is led to believe in https://bugs.llvm.org/show_bug.cgi?id=39754.) -- You are receiving this mail because: You are on the CC list for the bug. ___ bug-binutils mailing list bug-binutils@gnu.org https://lists.gnu.org/mailman/listinfo/bug-binutils
[Bug ld/24574] extern symbols in dlls are misleading under debugger
https://sourceware.org/bugzilla/show_bug.cgi?id=24574 Martin Storsjö changed: What|Removed |Added CC||martin at martin dot st --- Comment #7 from Martin Storsjö --- FWIW, I'm planning on looking into this in LLD, but if Eric is looking into it in binutils ld, I'd wait for progress on that first, as that's probably valuable info about what gdb requires to handle these symbols correctly. -- You are receiving this mail because: You are on the CC list for the bug. ___ bug-binutils mailing list bug-binutils@gnu.org https://lists.gnu.org/mailman/listinfo/bug-binutils
[Bug ld/24574] extern symbols in dlls are misleading under debugger
https://sourceware.org/bugzilla/show_bug.cgi?id=24574
--- Comment #9 from Martin Storsjö ---
I went ahead and dug into this a little bit more. First off, props for the
great testcase, it was great to even have the gdb test routine automated.
I haven't dug into the gdb sources to figure out exactly how/why it does
resolve those symbols, but the difference between what ld.bfd produces is
easily visible.
With ld.bfd before the referenced commit, the symbol table for the linked
test.dll looks like this:
$ x86_64-w64-mingw32-nm test.dll | grep pcode_st
6bec4260 R __fu0_pcode_st
6bec9300 I __imp_pcode_st
6bec9300 I __imp_pcode_st
6bec95ac I __nm_pcode_st
6bec4260 r .rdata$.refptr.pcode_st
6bec4260 R .refptr.pcode_st
Note that there's two symbols with the name __imp_pcode_st.
With ld.bfd after that referenced commit, the symbol table looks like this:
$ x86_64-w64-mingw32-nm test.dll | grep pcode_st
6bec4260 R __fu0_pcode_st
6bec9300 I __imp_pcode_st
6bec95ac I __nm_pcode_st
6bec9300 I pcode_st
6bec4260 r .rdata$.refptr.pcode_st
6bec4260 R .refptr.pcode_st
Now there's one symbol named __imp_pcode_st and one named pcode_st, but both
with the same address. The unprefixed symbol probably is what gbb (rightly)
picks up and considers the real address of the symbol, even if it points to the
import address table.
Likewise if LLD links against an import library generated by ld.bfd (LLD
doesn't support linking directly against DLLs), gdb also picks up the wrong
address of the symbol (using the IAT address instead), and the symbol table
contains this:
$ x86_64-w64-mingw32-nm test.dll | grep pcode_st
0001800038e0 R __imp_pcode_st
000180003b8c R __nm_pcode_st
0001800038e0 R pcode_st
0001800030b0 r .rdata$.refptr.pcode_st
0001800030b0 R .refptr.pcode_st
Now I can easily make LLD stop including the unprefixed symbol in the symbol
table, and then gdb manages to figure out the address of the symbol correctly.
I'll send a patch to LLD for this after I've made an accompanying mandatory
testcase, but the fix essentially looks like this:
diff --git a/COFF/Writer.cpp b/COFF/Writer.cpp
index 3da8b98d3..e157d6dff 100644
--- a/COFF/Writer.cpp
+++ b/COFF/Writer.cpp
@@ -1095,6 +1095,9 @@ Optional Writer::createSymbol(Defined
*def) {
}
}
+ if (def->isRuntimePseudoReloc)
+return None;
+
StringRef name = def->getName();
if (name.size() > COFF::NameSize) {
sym.Name.Offset.Zeroes = 0;
Even more surprisingly though, if I compile test.cpp with clang instead of gcc,
the gdb test succeeds, with both LLD (without this fix) and ld.bfd after the
change. Not sure if this stems from slightly different structure of .refptr and
similar structures, or different debug info that gdb manages to use better wrt
this.
--
You are receiving this mail because:
You are on the CC list for the bug.
___
bug-binutils mailing list
bug-binutils@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-binutils
[Bug ld/24574] extern symbols in dlls are misleading under debugger
https://sourceware.org/bugzilla/show_bug.cgi?id=24574 --- Comment #10 from Martin Storsjö --- The corresponding issue in LLD is fixed in https://reviews.llvm.org/D65727, both in trunk and in the upcoming 9.x release branch. -- You are receiving this mail because: You are on the CC list for the bug. ___ bug-binutils mailing list bug-binutils@gnu.org https://lists.gnu.org/mailman/listinfo/bug-binutils
[Bug ld/25374] linking to msvc import library results in invalid IDT entry
https://sourceware.org/bugzilla/show_bug.cgi?id=25374 Martin Storsjö changed: What|Removed |Added CC||martin at martin dot st --- Comment #1 from Martin Storsjö --- (In reply to Dan Raymond from comment #0) > Examination of the .idata section in the MinGW DLL revealed that ILT and IAT > tables were present for both the MSVC DLL and EXE. The IDT entry (which > contains pointers to the ILT and IAT) was correct for the MSVC DLL. The IDT > entry for the MSVC EXE, however, was incorrect. The ILT and IAT pointers > pointed to the NULL entries at the end of the tables rather than the first > entries. I was able to reproduce a similar issue when linking against two import libraries for two DLLs (without involving the concept of EXEs exporting symbols and import libraries for them). My suspicion is that the synthesized import table chunks (when using msvc style import libraries, where the linker needs to synthesize the import table contents) aren't sorted correctly if there's more than one msvc style import library involved. -- You are receiving this mail because: You are on the CC list for the bug.
[Bug binutils/27594] build processes broken by changed space handling
https://sourceware.org/bugzilla/show_bug.cgi?id=27594
Martin Storsjö changed:
What|Removed |Added
CC||martin at martin dot st
--- Comment #2 from Martin Storsjö ---
+1, there's predecent that windres takes this parameter as a
supposedly-pre-quoted/escaped string that is passed as-is to popen().
The same also goes for other preprocessor arguments, like -D and
--preprocessor-arg - they are passed almost (will elaborate below) as-is on to
popen(). This means that any caller that wants to pass e.g. a define for a
quoted string needs to escape quotes doubly - see e.g.
https://git.savannah.gnu.org/gitweb/?p=libiconv.git;a=blob;f=windows/windres-options;h=779fddec305d1e78f1e5c3123683b3c380e4a82e;hb=4b1a76b8e7f718fb23eb1a48cd1be208cfff6c2a
for an example of projects in the wild, doing extra escaping when passing
defines to windres.
A minimal example of that double quoting:
$ cat test1.rc
STRINGTABLE {
1 STRING1
}
$ x86_64-w64-mingw32-windres -DSTRING1=\"foo\" test1.rc
x86_64-w64-mingw32-windres: test1.rc:2: syntax error
$ x86_64-w64-mingw32-windres -DSTRING1=\\\"foo\\\" test1.rc
[..snip..]
STRINGTABLE MOVEABLE PURE DISCARDABLE
BEGIN
1, "foo"
END
Windres does try to escape one kind of char in the provided argument strings;
it tries to escape spaces with a backslash. Unfortunately, as the shells
(invoked by popen) differ between unix and windows (one can't backslash escape
spaces in windows command lines, the argument has to be quoted), this only
actually works when run on unix:
$ x86_64-w64-mingw32-windres "-DSTRING1=\\\"foo bar\\\"" test1.rc
[..snip..]
STRINGTABLE MOVEABLE PURE DISCARDABLE
BEGIN
1, "foo bar"
END
If you try to run the same on windows (built as mingw executables), it fails:
$ windres test1.rc "-DSTRING1=\\\"foo bar\\\""
gcc: error: bar": Invalid argument
C:\msys64\mingw64\bin\windres.exe: preprocessing failed.
Furthermore, other things that differ between how shells interpret things in
popen():
)$ x86_64-w64-mingw32-windres -DSTRING1=\\\"foobar\\\" test1.rc -v
Using `x86_64-w64-mingw32-gcc -E -xc -DRC_INVOKED -DSTRING1=\"foobar\"
test1.rc'
Using popen to read preprocessor output
[..snip..]
STRINGTABLE MOVEABLE PURE DISCARDABLE
BEGIN
1, "foo\\bar"
END
But on windows:
$ windres test1.rc -DSTRING1=\\\"foobar\\\" -v
[..snip..]
STRINGTABLE MOVEABLE PURE DISCARDABLE
BEGIN
1, "foobar"
END
Using `C:\msys64\mingw64\bin\gcc -E -xc -DRC_INVOKED -DSTRING1=\"foobar\"
test1.rc'
The last few are hypothetical. However the habit of double-escaping quotes for
such strings exists in the wild, and should be documented as such. The fact
that it differs between platforms exactly how one should do the double escaping
is problematic too, when it comes to more complicated strings.
(I recently implemented llvm-windres, and went to some lengths to mimic the GNU
windres behaviour of these options, to make it work as a drop-in replacement
for how existing projects in the wild call windres.)
--
You are receiving this mail because:
You are on the CC list for the bug.
[Bug binutils/27594] build processes broken by changed space handling
https://sourceware.org/bugzilla/show_bug.cgi?id=27594 --- Comment #9 from Martin Storsjö --- (In reply to Eli Zaretskii from comment #8) > Quoting with backslashes also doesn't work in cmd.exe. The suggestion to > pre-quote the preprocessor is also unreliable, because when `windres` is run > from cmd.exe or from the native MinGW Make, the program's startup code > unquotes the argument, so the result will depend on how many "unquoting" > levels the command line is subject to. I would beg to differ here; when any shell, be it either bash or cmd.exe, interprets a command line, it does split and unescape the command line depending on that shell's rules. It's true that the actual splitting and unescaping happens at different levels (in the caller, in the case of unix shells, and in the callee in the case of a win32 executable), but the basic principle is still the same; if I want to call a tool and have it receive a specific argument, I have to quote that argument according to the current shell's rules (and those rules differ with respect to certain details). So if I want to call windres so that it receives the argument "path to/gcc.exe" -E -DRC_INVOKED, I can call windres with "\"path to/gcc.exe\" -E -DRC_INVOKED" in cmd.exe and in bash - that particular quoting happens to work in both. In bash I could also call it with single quotes and not needing to escape the double quotes, as '"path to/gcc.exe" -E -DRC_INVOKED'. Thomas - the patch that was applied today, in https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=749c700282097cf679ff019a9674d7c762f48619, doesn't that adjust an entirely different thing? The quot() function isn't called for --preprocessor arguments, it's only called for --preprocessor-arg and -D and similar. It still just wraps the whole --preprocessor argument in double quotes, here: https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=binutils/windres.c;h=2b626a29fb68ada1cc012a267a60d72c1335bea9;hb=23182ac0d832477d316547ec2a758d22b43d0837#l889 I'm not arguing that what we have right now is pretty - it's not. But the core issue - that the --preprocessor argument used to accept multiple arguments (and where spaces would have to be pre-quoted) - do you want to willingly break that, which multiple existing projects rely on? I agree that the new interpretation of the option would be more straightforward to deal with - but it is a breaking change that affects multiple projects. I'd rather try to more clearly document the existing original behaviour and how to deal with it. Eli - separately, the new quoting behaviour (that was reverted now, needlessly?) is, in principle a good thing, but it's still missing to quote a few tricky cases - namely backslashes and double quotes. And fixing that would be breaking roughly a similar number of projects. If I want to preprocess with the option e.g. -DSTRING="quotedstring", I want windres to call popen with -DSTRING=\"quotedstring\", so I need to call windres with -DSTRING=\\\"quotedstring\\\". So it would in theory be great if quot() would handle proper escaping of backslashes and double quotes, but if we'd do that change, we'd be breaking those existing users that have codified that case of double escaping for such strings. -- You are receiving this mail because: You are on the CC list for the bug.
[Bug binutils/27594] build processes broken by changed space handling
https://sourceware.org/bugzilla/show_bug.cgi?id=27594 --- Comment #11 from Martin Storsjö --- (In reply to Eli Zaretskii from comment #10) > > So if I want to call windres so that it receives the argument "path > > to/gcc.exe" -E -DRC_INVOKED, I can call windres with "\"path to/gcc.exe\" -E > > -DRC_INVOKED" in cmd.exe and in bash - that particular quoting happens to > > work in both. In bash I could also call it with single quotes and not > > needing to escape the double quotes, as '"path to/gcc.exe" -E -DRC_INVOKED'. > > Here is where I disagree: the "\"path to/gcc.exe\" or '"path to/gcc.exe"' is > NOT quoting, it's double quoting. Quoting is just "path to/gcc.exe" or (for > Posix shells) 'path to/gcc.exe'. Yes, that's true: We need to make sure windres receives a pre-quoted string, thus the caller has to double quote it. > > The quot() function isn't called for --preprocessor > > arguments, it's only called for --preprocessor-arg and -D and similar. It > > still just wraps the whole --preprocessor argument in double quotes, here: > > https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=binutils/windres.c; > > h=2b626a29fb68ada1cc012a267a60d72c1335bea9; > > hb=23182ac0d832477d316547ec2a758d22b43d0837#l889 > > Yes, and thus I don't understand why the change to `quot` was reverted. If > that function's job is to quote something, then it should quote correctly on > MS-Windows, and the only sensible quoting on MS-Windows is "like this". If > someone wants to argue that arguments with whitespace should not be quoted, > then why do we still call `quot` on Unix? Indeed, I think that was a mistake in the revert. I'll try to follow up with a patch for reverting the right thing in a couple hours, and for clarifying the documentation regarding it - to codify the existing status quo regarding it. > > But the core issue - that the --preprocessor argument used to accept > > multiple arguments (and where spaces would have to be pre-quoted) - do you > > want to willingly break that, which multiple existing projects rely on? > > You just said that the change in `quot` doesn't affect --preprocessor, so > how can the change in `quot` break that (incorrect) usage of --preprocessor? > Why was it reverted? What am I missing here? Yeah the change in quot doesn't affect --preprocessor indeed. The "you" in this section was more of a generic towards the project, not you (singular) in particular. > > Eli - separately, the new quoting behaviour (that was reverted now, > > needlessly?) is, in principle a good thing, but it's still missing to quote > > a few tricky cases - namely backslashes and double quotes. And fixing that > > would be breaking roughly a similar number of projects. > > The code does attempt to quote double quotes, but if that code is buggy, > please show the specific use case where it breaks. If the code is > incomplete, let's fix that, by all means. > > As for backslashes: they don't need to be quoted on native Windows, only if > you are using a Posix shell, in which case the user should have typed the > original file name or string argument with backslashes already doubled, > right? > > > If I want to preprocess with the option e.g. -DSTRING="quotedstring", I want > > windres to call popen with -DSTRING=\"quotedstring\", so I need to call > > windres with -DSTRING=\\\"quotedstring\\\". > > I don't think I agree. The user should say -DSTRING="quotedstring" and the > code which invokes `popen` should then take care of quoting any arguments > that have embedded whitespace. Otherwise, we will have to know exactly how > many "unquoting" levels will the string undergo, and add quotes for each > level. That way lies madness, because it is not practical to do that -- you > never know how many such levels will the string undergo. The only sane way > is for the code which is going to pass a string to a shell command to quote > it if needed, and as appropriate to the command to be invoked. > > > So it would in theory be great > > if quot() would handle proper escaping of backslashes and double quotes, but > > if we'd do that change, we'd be breaking those existing users that have > > codified that case of double escaping for such strings. > > I still want to see those existing users which we will break. I suspect > that either they are already broken (and use loopholes due to bugs in the > implementation), or will simply be unaffected. > > So by all means, let's talk about those use cases one by one, and see what > we want to do about them. Yes I agree that the most sane thing to do would be to have the user specify -DSTRING="quotedstring", but there are projects that have ended up working around this, so fixing this case does break them. I'll follow up with a separate bug for that topic in a couple hours, with more examples and details, to avoid derailing this one further. -- You are receiving this mail because: You are on the CC list for the bug.
[Bug binutils/27594] build processes broken by changed space handling
https://sourceware.org/bugzilla/show_bug.cgi?id=27594
--- Comment #15 from Martin Storsjö ---
(In reply to Eli Zaretskii from comment #13)
> So bottom line: the revert reverted the wrong commit.
Yup, exactly.
(In reply to Eli Zaretskii from comment #12)
> That means `windres` will be the only program which requires such strange
> invocation rules. Every other program which does something similar doesn't
> require such "over-quoting".
Yes, there's no denying that it's an uncommon parameter definition.
> And I don't really understand why you are willing to require such strange
> rules: is that only because of some build scripts that use such broken
> multiple quoting? IOW, instead of fixing things (and the fixes are really
> simple), we are forever doomed to be bug-compatible with them? And only on
> Windows on top of that?
None of this is Windows specific; you've been able to call it with
--preprocessor "gcc -xc -E -DRC_INVOKED ..." in the same way on both unix and
Windows for the last 10 years. And if one wanted to specify the tool with a
path with spaces (prior to the 2.36 release), that path would have to be double
quoted - both on unix and Windows. (Admittedly, paths with spaces are much
rarer on unix.)
Regarding arguing for/against this, I think existing cases where projects are
using --preprocessor to pass multiple arguments are much much more common than
cases where one wants to specify the executable with a path that needs to be
quoted. And by going with the historical behaviour of the option, those users
are left working, and there's still one way (which is uncomfortable though) to
pass paths with spaces.
In many cases, it's indeed simple to rewrite things in a way that works with
both old and new windres. In one case,
https://git.videolan.org/?p=ffmpeg.git;a=blob;f=ffbuild/common.mak;h=32f5b997b5979c9799aafdf88d84fb4b6677ec80;hb=f8d910e90f599f338438833dfc92e2f1915ce414#l93,
a makefile variable containing a number of preprocessor options (in most cases
something like "-MMD -MF -MT ") is passed in that way;
rewriting that to use --preprocessor-arg requires a bit more changes.
I'm not dead set against changing it, but if changed, I would prefer that it
isn't swept under the rug as "all these users were misusing the option" but own
up to that the old usage was a seemingly valid way of using the option (the
documentation almost hinted that it was supposed to be used that way) and we're
willingly breaking those users for a clearer definition of the option.
> > Yes I agree that the most sane thing to do would be to have the user specify
> > -DSTRING="quotedstring", but there are projects that have ended up working
> > around this, so fixing this case does break them.
>
> Those projects should get their act together, if you ask me. It isn't hard,
> really. Not my call, obviously, but still...
Not at all actually, so far (in releases) there was no way of doing that that
didn't involve double quoting. But that's for a separate bug report.
(In reply to Eli Zaretskii from comment #14)
> And one more comment: using pre-quoted shell command (as opposed to just the
> preprocessor file name) in --preprocessor will not work with
> --use-temp-file, because the code in `run_cmd` breaks the command into
> argv[] array, and will treat the quoted string ("gcc -E ") as a file
> name to invoke. This trick only works in the default pipe mode, because
> then the command is not broken into argv[] elements.
No, I don't think so. Before 2.36, a pre-quoted argument in --preproessor would
have ended up as <<<"path to/gcc.exe" -xc -E ...>>> in the cmd string, and
run_cmd would split it correctly. (In 2.36, such arguments to --preprocessor
don't work though.)
The command string, assembled from --preprocessor arguments, -D/-I and other
options, either gets split into an argv[] array by run_cmd, or passed as such
to popen() (where the shell does the same splitting), and arguments with spaces
in the string need to be quoted the same in both cases.
So the cmd string (which is concatenated from the output of the 'quot'
function, and the 'quot' function quotes things in a platform specific way) is
defined to be escaped/quoted for the current platform shell - and the 'run_cmd'
function would need to mirror those platform specific quoting nuances (e.g.
regarding backslash/quote escaping).
--
You are receiving this mail because:
You are on the CC list for the bug.
[Bug binutils/27843] New: windres quoting of backslashes and double quotes is lacking
https://sourceware.org/bugzilla/show_bug.cgi?id=27843
Bug ID: 27843
Summary: windres quoting of backslashes and double quotes is
lacking
Product: binutils
Version: unspecified
Status: UNCONFIRMED
Severity: normal
Priority: P2
Component: binutils
Assignee: unassigned at sourceware dot org
Reporter: martin at martin dot st
Target Milestone: ---
Given a rc file like this:
$ cat test.rc
STRINGTABLE {
1 STRING
}
Normally, if one were to provide the definition of STRING via a command line
option when preprocessing C, one would specify it as -DSTRING=\"foo\" on the
command line, so that the tool receives the argument as -DSTRING="foo", and
substitutes "foo".
So far, across all versions (modern at least) of windres, the quotes aren't
re-escaped properly. So currently, the only way of getting this to work is to
double escape the backslashes, into this form, that works on both unix and
windows:
$ x86_64-w64-mingw32-windres test.rc test.res -DSTRING=\\\"foo\\\" -v
Using `x86_64-w64-mingw32-gcc -E -xc -DRC_INVOKED -DSTRING=\"foo\" test.rc'
Currently (after cc3edc52747fd8b184ee48f1b0cc1ac0aca7832e, up until the revert
mishap in the current master version) it actually does work without double
escaping (on windows only), if the string otherwise happens to contain a space:
> windres.exe test.rc test.res "-DSTRING=\"fo o\"" -v
Using `mingw64\bin\gcc -E -xc -DRC_INVOKED -D"STRING=\"fo o\"" test.rc'
But without a space:
> windres.exe test.rc test.res -DSTRING=\"foo\" -v
Using `mingw64\bin\gcc -E -xc -DRC_INVOKED -DSTRING="foo" test.rc'
Using popen to read preprocessor output
windres.exe: test.rc:2: syntax error
So double quotes need to be escaped, on all platforms, even if the string
doesn't contain spaces.
Fixing this is going to break GNU libiconv, which contains a script for doing
double escaping of arguments to windres:
https://git.savannah.gnu.org/gitweb/?p=libiconv.git;a=blob;f=windows/windres-options;h=779fddec305d1e78f1e5c3123683b3c380e4a82e;hb=4b1a76b8e7f718fb23eb1a48cd1be208cfff6c2a
And the script is unconditionally called with the --escape option for enabling
the escaping:
https://git.savannah.gnu.org/gitweb/?p=libiconv.git;a=blob;f=src/Makefile.in;h=7035316ae52a38fd7515e065a63a5c14ee7f4efb;hb=4b1a76b8e7f718fb23eb1a48cd1be208cfff6c2a#l87
If this is fixed, I don't offhand see a god way of fixing iconv to be buildable
both with old and new versions of windres at the same time, short of either
trying both quoting forms or just rewriting the use of those macros to avoid
needing to pass quoted strings on the command line.
Additionally, once the quotes are escaped/quoted properly, other tricky strings
that may need more work for quoting (with different quoting styles for windows
and unix) are strings that contain single or double backslashes (not directly
before a double quote). (I don't believe there's real use of this in the wild,
but would be good to handle correctly for consistency.)
--
You are receiving this mail because:
You are on the CC list for the bug.
[Bug binutils/27843] windres quoting of backslashes and double quotes is lacking
https://sourceware.org/bugzilla/show_bug.cgi?id=27843 Martin Storsjö changed: What|Removed |Added CC||eliz at gnu dot org -- You are receiving this mail because: You are on the CC list for the bug.
[Bug binutils/27594] build processes broken by changed space handling
https://sourceware.org/bugzilla/show_bug.cgi?id=27594 --- Comment #19 from Martin Storsjö --- (In reply to Eli Zaretskii from comment #18) > Moreover, `windres` calls `popen` only in its default mode; you could change > that with the --use-temp-file option, in which case the preprocessor will be > invoked via `pexecute`, and that has a different set of > implementation-defined perculiarities wrt quoted parts of the command. When windres calls pexecute, windres itself splits the string (which is in a quoted/escaped form suitable for passing to popen) in the run_cmd function, so run_cmd is supposed to split/unescape things in the same way as the shell would. So ideally, the distinction between those two souldn't be visible externally. > > Whether the wrong commit was reverted or not I cannot judge right now, I was > > only asking to restore the 2.35 behaviour which is consistent with general > > POSIX string handling and which supports everything anyone needs. Breaking > > such support after years is what's generally called a regression and it's > > generally and rightfully frowned upon. > > I disagree here as well. If someone used the tool incorrectly, in direct > contradiction to its docs, then backward compatibility doesn't apply, IMO. This is indeed the core of the issue. Up until the 2.35 version, the option was documented as "This option may be used to specify the preprocessor to use, including any leading arguments. The default preprocessor argument is `gcc -E -xc-header -DRC_INVOKED`." I think that's rather clearly saying that this kind of behaviour was explicitly intended and allowed? -- You are receiving this mail because: You are on the CC list for the bug.
[Bug binutils/27594] build processes broken by changed space handling
https://sourceware.org/bugzilla/show_bug.cgi?id=27594 --- Comment #21 from Martin Storsjö --- (In reply to Eli Zaretskii from comment #20) > (In reply to Martin Storsjö from comment #19) > > Up until the 2.35 version, the option was documented as "This option may be > > used to specify the preprocessor to use, including any leading arguments. > > The default preprocessor argument is `gcc -E -xc-header -DRC_INVOKED`." I > > think that's rather clearly saying that this kind of behaviour was > > explicitly intended and allowed? > > So the incompatible change is in the documentation, right? The code just > does what the documentation says it should, right? No; in 21c33bcbe36377abf01614fb1b9be439a3b6de20 (Nov 2020) the implementation was changed to put full quotes around the string coming from --preprocessor; this broke passing extra leading arguments (as previously was documented as supported). In practice, popen() on windows might still have worked in some cases, but it no longer did on unix (and cygwin). In 5edb8e3f5ad8d74a83fc0df7f6e4514eed0aa77f (Apr 2021) the documentation was changed to stop saying that "the default preprocessor argument is `gcc -E -xc-header -DRC_INVOKED`". In 749c700282097cf679ff019a9674d7c762f48619 (May 2021), which was supposed to restore the original --preprocessor behaviour, it further removed the statement that the --preprocessor option can be used to specify other leading arguments to the preprocessor. -- You are receiving this mail because: You are on the CC list for the bug.
[Bug binutils/27594] build processes broken by changed space handling
https://sourceware.org/bugzilla/show_bug.cgi?id=27594 --- Comment #24 from Martin Storsjö --- (In reply to Nick Clifton from comment #22) > To be clear, my opinion is that previously the documentation was incorrect. > I do not believe that it was ever the intention that the --preprocessor > option should also accept command line options. It did, yes, and in fact it > still does, but it should not be relied upon. Arguments to the preprocessor > should always be passed via the --preprocessor-arg option. This is indeed the core of the issue. I can't speculate on what the original intention of the option was, but the docs did say explicitly "including any leading arguments", and then also went on to mention the default option value including extra arguments. And as this documentation matched how it behaved in practice, I'd say it has been hard for users to guess that the intent of the option to be used that way. I guess the alternative is to more clearly say that we're intentionally changing (or, already changed) the behaviour to clarify and simplify it, and are accepting some collateral breakage. It's primarily the faulting of the users for doing what the docs said, that I'm objecting to. (In reply to Eli Zaretskii from comment #23) > then at least commit > 749c700282097cf679ff019a9674d7c762f48619 should be reverted, as it changed > the function `quot` which is not related to handling of --preprocessor. Can > we at least agree on this latter point? Yes, full agree on that part. -- You are receiving this mail because: You are on the CC list for the bug.
[Bug ld/25374] linking to msvc import library results in invalid IDT entry
https://sourceware.org/bugzilla/show_bug.cgi?id=25374 --- Comment #4 from Martin Storsjö --- The issue shown in the reproduction sample posted by Petteri seems to have been caused by a regression in binutils 2.34, that was fixed again in binutils 2.35. The commit that caused the breakage was this one: https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=90d92a632aaf75ce698335efeb383ddf785c12d8 The issue was resolved later by this commit: https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=5e365e474b7561318ddb1a107f05cf0c002e8284 It's possible that this also was the same issue that this bug was filed about, but it could also maybe have been a different issue. I'm not able to reproduce any bug similar to this with current versions of binutils though. -- You are receiving this mail because: You are on the CC list for the bug.
[Bug ld/25541] linking twice to the same msvc import library produces an invalid IAT
https://sourceware.org/bugzilla/show_bug.cgi?id=25541 --- Comment #1 from Martin Storsjö --- Created attachment 13682 --> https://sourceware.org/bugzilla/attachment.cgi?id=13682&action=edit Attempt at reproducing the issue The explanation of the issue sounds indeed like something that could happen, but I tried reproducing it (with a manually crafted minimal set of sources) and wasn't able to. I'm attaching the set of sources I tried to use for reproducing the issue. Maybe it's possible to trigger it if the individual source files link in other system functions, before trying to import things from that same import lib another time - I also tried with a more extensive example that does that, but didn't still manage to reproduce it. So if this issue still is reproducible with binutils >= 2.35, then it'd be great if you can share a bunch of prebuilt object files/import libs/static libs that can be used for reproducing and analyzing the issue. -- You are receiving this mail because: You are on the CC list for the bug.
[Bug binutils/12633] Crash with dlltool geneated import *.libs in MSVC++ Release target (Debug OK)
http://sourceware.org/bugzilla/show_bug.cgi?id=12633 --- Comment #2 from Martin Storsjö 2011-05-02 11:26:59 UTC --- Created attachment 5700 --> http://sourceware.org/bugzilla/attachment.cgi?id=5700 Minimal test case Here's an attached quite minimal testcase for this issue. The makefile builds a tiny DLL using mingw/gcc, and generates an import library, both using --out-implib with ld and using dlltool. Then the makefile builds two test exes that links to the created import libraries - both exes run just fine on windows. The batch script msvc.bat builds the same test exes using the MSVC command line tools, both with and without the /OPT:REF flag, against both of the created import libraries. The ones built with /OPT:NOREF work fine, while the ones built with /OPT:REF fail. For comparison, it also creates an import library from the .def file using lib.exe, and links against it. For the exes built against this import library, both versions work. -- Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ bug-binutils mailing list bug-binutils@gnu.org https://lists.gnu.org/mailman/listinfo/bug-binutils
