1186934 - clang-cl error LNK2005: _je_malloc_conf already defined

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(Away)]

I de-unified jemalloc to make things more clear:

 4:30.96 Executing: link -NOLOGO -DLL -OUT:replace_jemalloc.dll -PDB:replace_jemalloc.pdb -SUBSYSTEM:WINDOWS,5.01 -MACHINE:X86 @C:\build\msys\s\central\obj\clang\memory\replace\jemalloc\tmpghipfi.list module.res -DYNAMICBASE -DEBUG -DEBUGTYPE:CV -DEBUG -OPT:REF kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib secur32.lib netapi32.lib
 4:30.96 C:\build\msys\s\central\obj\clang\memory\replace\jemalloc\tmpghipfi.list:
 4:30.96     jemalloc_config.obj
 4:30.96     mozjemalloc_compat.obj
 4:30.96     ..\..\jemalloc\arena.obj
 4:30.96     ..\..\jemalloc\atomic.obj
 4:30.96     ..\..\jemalloc\base.obj
 4:30.96     ..\..\jemalloc\bitmap.obj
 4:30.96     ..\..\jemalloc\chunk.obj
 4:30.96     ..\..\jemalloc\chunk_dss.obj
 4:30.96     ..\..\jemalloc\chunk_mmap.obj
 4:30.96     ..\..\jemalloc\ckh.obj
 4:30.96     ..\..\jemalloc\ctl.obj
 4:30.96     ..\..\jemalloc\extent.obj
 4:30.96     ..\..\jemalloc\hash.obj
 4:30.96     ..\..\jemalloc\huge.obj
 4:30.96     ..\..\jemalloc\jemalloc.obj
 4:30.96     ..\..\jemalloc\mb.obj
 4:30.96     ..\..\jemalloc\mutex.obj
 4:30.96     ..\..\jemalloc\prof.obj
 4:30.96     ..\..\jemalloc\quarantine.obj
 4:30.96     ..\..\jemalloc\rtree.obj
 4:30.96     ..\..\jemalloc\stats.obj
 4:30.96     ..\..\jemalloc\tcache.obj
 4:30.96     ..\..\jemalloc\tsd.obj
 4:30.96     ..\..\jemalloc\util.obj
 4:30.96 
 4:30.96 jemalloc.obj : error LNK2005: _je_malloc_conf already defined in jemalloc_config.obj

I can see the definitions of je_malloc_conf in memory/replace/jemalloc_config.c and memory/jemalloc/src/src/jemalloc.c.

And, if I build with Visual Studio and open those .obj files with a hex editor, I see _je_malloc_conf mentioned in both of them.

So why does this not produce the same "already defined" error with VS?

Comment 1

[(Away)]

I don't really know what I'm doing here, but here are some differences in the |dumpbin -all| outputs of jemalloc.obj from the two compilers (no differences stood out to me in the jemalloc_config.obj files).

It seems clang-cl has a section for _je_malloc_conf vs UNDEF for VS2013. And the linker directives are different.

VS2013 -------------------------------------------------------------------

RAW DATA #1
...
   Linker Directives
   -----------------
   /DEFAULTLIB:MSVCRT
   /DEFAULTLIB:OLDNAMES
   /EXPORT:_je_malloc_conf,DATA
   /EXPORT:_replace_malloc
   /EXPORT:_replace_calloc
   /EXPORT:_replace_posix_memalign
   /EXPORT:_replace_aligned_alloc
   /EXPORT:_replace_realloc
   /EXPORT:_replace_free
   /EXPORT:_je_mallocx
   /EXPORT:_je_rallocx
   /EXPORT:_je_xallocx
   /EXPORT:_je_sallocx
   /EXPORT:_je_dallocx
   /EXPORT:_je_sdallocx
   /EXPORT:_je_nallocx
   /EXPORT:_je_mallctl
   /EXPORT:_je_mallctlnametomib
   /EXPORT:_je_mallctlbymib
   /EXPORT:_je_malloc_stats_print
   /EXPORT:_replace_malloc_usable_size
   /EXPORT:_replace_memalign
   /EXPORT:_replace_valloc

COFF SYMBOL TABLE
...
008 00000004 UNDEF  notype       External     | _je_malloc_conf


clang-cl -----------------------------------------------------------------

RAW DATA #67
...
   Linker Directives
   -----------------
   /DEFAULTLIB:msvcrt.lib
   /DEFAULTLIB:oldnames.lib


COFF SYMBOL TABLE
...
1A2 00000050 SECT3  notype       External     | _je_malloc_conf

Comment 2

[(no longer active)]

clang doesn't know how to generate /EXPORT linker directives at all.  We should figure out where those come from, and see if we need to add support for them to clang, or change our code to just use __declspec(dllexport).

Comment 3

[(Away)]

> clang doesn't know how to generate /EXPORT linker directives at all.
But it appears to work for jemalloc_conf.obj?

VS2013 -------------------------------------------------------------------

RAW DATA #1
...
   Linker Directives
   -----------------
   /DEFAULTLIB:MSVCRT
   /DEFAULTLIB:OLDNAMES
   /EXPORT:_je_malloc_conf,DATA

COFF SYMBOL TABLE
...
00A 00000000 SECT4  notype       External     | _je_malloc_conf

clang-cl -----------------------------------------------------------------

RAW DATA #7
...
   Linker Directives
   -----------------
   /DEFAULTLIB:msvcrt.lib
   /DEFAULTLIB:oldnames.lib
   /EXPORT:_je_malloc_conf,DATA

COFF SYMBOL TABLE
...
022 00000000 SECT2  notype       External     | _je_malloc_conf

Comment 4

[(Away)]

Attachment: Rearrange jemalloc_macros ifdefs

I'm not sure if this is the right fix, but it seems to let my build get further.

Comment 5

[(Away)]

Comment on attachment 8638083 [details] [diff] [review]
Rearrange jemalloc_macros ifdefs

Review of attachment 8638083 [details] [diff] [review]:
-----------------------------------------------------------------

Am I on the right track here?

Comment 6

[(no longer active)]

(In reply to David Major [:dmajor] from comment #3)
> > clang doesn't know how to generate /EXPORT linker directives at all.
> But it appears to work for jemalloc_conf.obj?

Oh, so previously I grepped through the clang source code, but testing it out, it seems like this is how __declspec(dllexport) is implemented, so please ignore my previous comment.  :-)

$ cat test.cpp
__declspec(dllexport) void foo() {}
$ ./bin/clang-cl  -c test.cpp
$ ./bin/llvm-objdump -s test.obj

test.obj:	file format COFF-x86-64

Contents of section .text:
 0000 c3                                   .
Contents of section .drectve:
 0000 202f4445 4641554c 544c4942 3a6c6962   /DEFAULTLIB:lib
 0010 636d742e 6c696220 2f444546 41554c54  cmt.lib /DEFAULT
 0020 4c49423a 6f6c646e 616d6573 2e6c6962  LIB:oldnames.lib
 0030 202f4558 504f5254 3a3f666f 6f404059   /EXPORT:?foo@@Y
 0040 4158585a                             AXXZ
Contents of section .xdata:
 0000 01000000 00000000                    ........
Contents of section .pdata:
 0000 00000000 01000000 00000000           ............

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8638083 [details] [diff] [review]
Rearrange jemalloc_macros ifdefs

Review of attachment 8638083 [details] [diff] [review]:
-----------------------------------------------------------------

This makes sense. Please add a patch file, and update memory/jemalloc/update.sh. Please also file upstream (note that the file changed upstream so the patch won't apply as-is)

Comment 8

[(Away)]

For what it's worth, I've been mostly ignoring this bug since for asan builds (the main motivation for clang-cl) we'll need to disable jemalloc anyway.

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #7)
> This makes sense. Please add a patch file, and update
> memory/jemalloc/update.sh. Please also file upstream (note that the file
> changed upstream so the patch won't apply as-is)

Is upstream here your jemalloc repo or the "real" upstream repo?  If it gets accepted to "real" upstream, I take it a cherry-picked pull request should be made against your repo, and memory/jemalloc/upstream.info updated appropriately?

Comment 10

[Nathan Froyd [:froydnj]]

Bother, ni? for comment 9.

Comment 11

[Mike Hommey [:glandium]]

Mmmm my original comment dates from before I got rid of all the local patches and used a separate repository. The separate repository was meant to be a temporary thing, though, and I'm hoping that the remaining thing that is not merged yet will be soon. So if we want to land this before it makes it to the real upstream, let's still go with the old local patch thing.

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #11)
> Mmmm my original comment dates from before I got rid of all the local
> patches and used a separate repository. The separate repository was meant to
> be a temporary thing, though, and I'm hoping that the remaining thing that
> is not merged yet will be soon. So if we want to land this before it makes
> it to the real upstream, let's still go with the old local patch thing.

This has made it upstream:

https://github.com/jemalloc/jemalloc/commit/566d4c02400700b94a952eddeed34313360211d3

What now?

Comment 13

[Mike Hommey [:glandium]]

Considering the last piece from my branch made it upstream too, we can update to current dev tip.

Comment 14

[Nathan Froyd [:froydnj]]

Attachment: update jemalloc to upstream HEAD

This patch just followed the instructions in README.mozilla.  It looks like
there's been some upstream fixes to support unified compilation, so we can turn
that on in a separate bug.

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 8698020 [details] [diff] [review]
update jemalloc to upstream HEAD

Review of attachment 8698020 [details] [diff] [review]:
-----------------------------------------------------------------

FWIW, the runstatedir-related differences in configure seem to come from the difference in autoconf version you're using vs. mine (not that it matters much).

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/303cefb1809a

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/31a4f341fc38

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/303cefb1809a
https://hg.mozilla.org/mozilla-central/rev/31a4f341fc38