1500804 - Cut nsis build over from mingw-gcc to mingw-clang

Status: RESOLVED FIXED

(Firefox Build System :: General: Unsupported Platforms, enhancement, P5)

Comment 1

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Worked on this a bit.  I'm cutting over https://searchfox.org/mozilla-central/source/taskcluster/scripts/misc/build-mingw32-nsis.sh for reference.

1) zlib needs a simple patch in win32/Makefile.gcc to specify clang instead of gcc. Easy.

2) zlib uses windres, which seems to have some sort of hardcoded reference to gcc?  symlinking i686-w64-mingw32-gcc to i686-w64-mingw32-clang succeeds.  (This one took a bit to figure out since I had two i686-w64-mingw32-gcc's hiding in my series of $PATHs on my local machine!)

3) zlib also calls strip; which fails because it can't find i686-w64-mingw32-strip. STRIP=/bin/true (per Bug 1471698) works

4) nsis is quite wonky. It also has requirements to use 'gcc' and not 'clang'; but symlinking gcc and g++ to clang and clang++ gets past that issue.

5) In nsis-3.01-src/SCons/Config/gnu I needed to remove -Wl,--file-alignment,512 (lld doesn't recognize it)

6) I also got errors with -Wl,--exclude-libs,msvcrt.a ; but removing that gave me the error lld-link: error: undefined symbol: _memset

At this point I decided it would be best to get some feedback whenever it's available.

Comment 2

[Martin Storsjö]

(In reply to Tom Ritter [:tjr] from comment #1)
> Worked on this a bit.  I'm cutting over
> https://searchfox.org/mozilla-central/source/taskcluster/scripts/misc/build-
> mingw32-nsis.sh for reference.
> 
> 1) zlib needs a simple patch in win32/Makefile.gcc to specify clang instead
> of gcc. Easy.
> 
> 4) nsis is quite wonky. It also has requirements to use 'gcc' and not
> 'clang'; but symlinking gcc and g++ to clang and clang++ gets past that
> issue.

Yes, generally when compiling code, exposing clang under gcc/g++ aliases helps a lot to make code compile without changes. This is also what Apple have been doing in Xcode since clang became the default there.

> 2) zlib uses windres, which seems to have some sort of hardcoded reference
> to gcc?  symlinking i686-w64-mingw32-gcc to i686-w64-mingw32-clang succeeds.
> (This one took a bit to figure out since I had two i686-w64-mingw32-gcc's
> hiding in my series of $PATHs on my local machine!)

Yes, windres uses a C compiler to preprocess the rc files before parsing them, and the name is hardcoded in the windres binary.

So I take it you have a binutils based windres available here?

> 3) zlib also calls strip; which fails because it can't find
> i686-w64-mingw32-strip. STRIP=/bin/true (per Bug 1471698) works

If you have windres from binutils, can't you just provide a matching strip executable from there as well?

> 5) In nsis-3.01-src/SCons/Config/gnu I needed to remove
> -Wl,--file-alignment,512 (lld doesn't recognize it)

Indeed, lld doesn't support this flag. Internally lld first mimics link.exe, and link.exe has got a /filealign flag, so if needed, it should be straightforward to add support for the /filealign flag first in lld-link and the have the ld.lld mingw frontend convert --file-alignment into /filealign.

But I'm pretty sure this isn't necessary, since the default file alignment is 512 in both lld and binutils ld.

> 6) I also got errors with -Wl,--exclude-libs,msvcrt.a ;

This flag is about avoiding automatically exporting symbols from specific libraries linked in. MinGW tools automatically export all symbols when creating a DLL, if no dllexport attributes were found. By default it tries to avoid exporting symbols from system libraries that are linked in statically, but you can also pass --exclude-libs to explicitly avoid this. Both lld and binutils ld avoid exporting anything from msvcrt by default, so unless this specifically means a library named "msvcrt.a" (contrary to the usual "libmsvcrt.a"), this also just reinforces the defaults and aren't necessary. I haven't implemented these options in lld as I haven't run into a concrete need so far.

> but removing that gave me the error lld-link: error: undefined symbol: _memset

This shouldn't be related to either of the points you listed above; I'd need to see the full link command. I guess it sounds like it doesn't have the `-lmsvcrt` as normally is injected by the compiler driver. Is something setting `-nodefaultlibs` or `-nostdlib` when linking?

Comment 3

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: SCons/Config/linker_script

Comment 4

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: full linker command

(In reply to Martin Storsjö from comment #2)
> > but removing that gave me the error lld-link: error: undefined symbol: _memset
> 
> This shouldn't be related to either of the points you listed above; I'd need
> to see the full link command. I guess it sounds like it doesn't have the
> `-lmsvcrt` as normally is injected by the compiler driver. Is something
> setting `-nodefaultlibs` or `-nostdlib` when linking?

Yes, -nostdlib.  Removing that gives 
> lld-link: error: undefined symbol: _WinMain@16
> >>> referenced by libmingw32.a(lib32_libmingw32_a-crt0_c.o):(_main)

The full command is attached, and I also included the linker script.

Comment 5

[Martin Storsjö]

(In reply to Tom Ritter [:tjr] from comment #4)
> Created attachment 9019416 [details]
> full linker command
> 
> (In reply to Martin Storsjö from comment #2)
> > > but removing that gave me the error lld-link: error: undefined symbol: _memset
> > 
> > This shouldn't be related to either of the points you listed above; I'd need
> > to see the full link command. I guess it sounds like it doesn't have the
> > `-lmsvcrt` as normally is injected by the compiler driver. Is something
> > setting `-nodefaultlibs` or `-nostdlib` when linking?
> 
> Yes, -nostdlib.  Removing that gives 
> > lld-link: error: undefined symbol: _WinMain@16
> > >>> referenced by libmingw32.a(lib32_libmingw32_a-crt0_c.o):(_main)
> 
> The full command is attached, and I also included the linker script.

Ah, ok, now I think I understand. This really is intended to be linked with -nostdlib and isn't intended to use any of the CRT functions. However, clang has spontaneously generated a call to memset somewhere. Building whichver object file that contained the memset reference with -fno-builtin-memset might help with that.

Comment 6

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: memset repro.c

-fno-builtin-memset and -nostdlib is working to get it going, although I haven't yet figured out how to uniformly apply the flags via the config file.  That part might be a pile of hacks.

However I eventually hit something those flags didn't fix.

Using this toolchain (I think): https://queue.taskcluster.net/v1/task/CQO4iDh2RraPuA2ttxTXVg/runs/0/artifacts/public/build/clangmingw.tar.xz

Command:
> i686-w64-mingw32-gcc -o repro.o -c -Os -Wall -fno-strict-aliasing -nostdlib -fno-builtin-memset -DNSISCALL="__attribute__((__stdcall__))" -D_UNICODE -DUNICODE repro.c

Reproduction file attached.  This produces an object file that imports memset, and subsequently causes the build to fail:
> llvm-nm repro.o | grep memset
>          U _memset

Comment 7

[Martin Storsjö]

I've reduced the issue down to this:

$ cat repro.c
void other(unsigned short *ptr);
void func(void) {
  unsigned short buf[128] = L"";
  other(buf);
}
$ clang -target x86_64-w64-mingw32 -c repro.c -Os -ffreestanding -fno-builtin
$ llvm-nm repro.o 
00000000 a @feat.00
00000000 T func  
         U memset
         U other

The key seems to be in -Os, with -O2 it doesn't happen. Short term workaround would be to either change to any other optimization level than -Os, or to provide a manual implementation of memset somewhere when building this, e.g. something like this:

void *memset(void *b, int c, size_t len) {
    char *ptr = b;
    for (size_t i = 0; i < len; i++)
        ptr[i] = c;
    return b;
}

The issue in itself is that clang produces IR with a call to the intrinsic llvm.memset for doing the clearing. Depending on a number of factors, the llvm.memset intrinsic gets lowered to different things. Not sure if the -fno-builtin and -ffreestanding options are supposed to have any effect on that level, or if there's some different option at that level that would affect it - I'll poke around, ask people and/or file a bug on LLVM about it.

Comment 8

[Martin Storsjö]

I asked around clang/LLVM, and apparently this isn't necessarily a bug. The behaviour intends to match GCC, where the documentation says this:

https://gcc.gnu.org/onlinedocs/gcc/Standards.html#Standards

> GCC requires the freestanding environment provide memcpy, memmove, memset and memcmp.

That is, no matter how much one tries to ask the compiler not to, the compiler is free to produce calls to these functions anywhere. So if linking with -nostdlib, the environment more or less needs to provide these functions.

So given that, it's mostly a case of differing optimizations, where GCC hasn't chosen to use memset (even if it would be allowed to) while LLVM chooses to use it, when optimizing for size.

So I guess the only options are to not compile this code with -O2 or to add an extra implementation of memset somewhere.

Comment 9

[Jacek Caban]

I can see in logs that nsis provides its own memset implementation (and passes SCons/Config/memset.c to the link command - note that it passes .c file, not an object file). Since we use clang++ as a linker, it decides to build it in c++ mode and mangles memset name. I think that adding extern "C" to memset implementation should fix the problem.

Comment 10

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Per conversation with :kmoir, I'm going through untriaged bugs in her components and marking the ones which look to be enhancements/tasks with the enhancement severity to get them out of the triage queue.

If this incorrect, please remove the tag.

Comment 11

[Kim Moir [:kmoir] ET]

triaging, :tjr is this a bug you'd like to own? I notice you have been working on it for a while

Comment 12

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

I expect to be the one to do it; so I can take it, yeah.

Comment 13

[(Away)]

Came across this bug while I was trying to remember whether we still care about mingw-gcc or not.

Tom, did you ever get a chance to try Jacek's comment 9?

Comment 14

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

(In reply to :dmajor from comment #13)

Came across this bug while I was trying to remember whether we still care about mingw-gcc or not.

Tom, did you ever get a chance to try Jacek's comment 9?

I'm afraid this is so paged out in my mind that I don't recall if I tried it or if it worked.

Comment 15

[Mike Hommey [:glandium]]

The memset undefine symbol thing comes from:

clang-8: warning: treating 'c' input as 'c++' when in C++ mode, this behavior is deprecated [-Wdeprecated]

Because it compiles memcpy.c and memset.c as C++, the symbols are C++-mangled, so unmangled memset/memcpy can't be found.

Comment 16

[Mike Hommey [:glandium]]

Attachment: Bug 1500804 - Build nsis with mingw-clang rather than mingw-gcc.

While here, build the Linux portions with a sysroot.

Comment 17

[Mike Hommey [:glandium]]

Attachment: Bug 1500804 - Remove now unused mingw32-gcc toolchain.

Comment 18

[Phabricator Automation]

Comment on attachment 9241730 [details]
Bug 1500804 - Remove now unused mingw32-gcc toolchain.

Revision D125923 was moved to bug 1500802. Setting attachment 9241730 [details] to obsolete.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/69c80d34d66e
Build nsis with mingw-clang rather than mingw-gcc. r=tjr

Comment 20

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/69c80d34d66e