llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
27.04k stars 11.07k forks source link

Linkerror with __declspec(selectany) (multiple definitions) #32632

Open llvmbot opened 7 years ago

llvmbot commented 7 years ago
Bugzilla Link 33285
Version trunk
OS Linux
Reporter LLVM Bugzilla Contributor
CC @DougGregor,@mstorsjo,@zygoloid,@rnk

Extended Description

I get error from the linker when compiling ChakraCore https://github.com/Microsoft/ChakraCore with clang trunk. I happen with ld.gold and ld.lld. Surprisingly it doesn't happen when compiling with clang-3.8, so clearly there is a regression.

The link error is: /usr/bin/ld: error: duplicate symbol: BVUnitT::ShiftValue

defined at /home/prazek/devirtualization-results/ChakraCore/bin/GCStress/GCStress.cpp bin/GCStress/CMakeFiles/GCStress.dir/GCStress.cpp.o:(BVUnitT::ShiftValue) defined at /home/prazek/devirtualization-results/ChakraCore/lib/Common/Memory/HeapAllocator.cpp HeapAllocator.cpp.o:(.rodata+0x4) in archive lib/libChakraCoreStatic.a

This template is defined here https://github.com/Microsoft/ChakraCore/blob/master/lib/Common/DataStructures/UnitBitVector.h#L491

The reproduced is simple:

  1. git clone git@github.com:Microsoft/ChakraCore.git
  2. cd ChakraCore
  3. ./build.sh -n --cxx=[pathToBuildDir]/bin/clang++ --cc=[pathToBuildDir]/bin/clang
llvmbot commented 6 years ago

I tried following the instructions, but it fails with error: invalid stable version

for command: utils/release/merge-request.sh -stable-version 5.0.1 -r 313278 -user piotr.padlewski@gmail.com

mstorsjo commented 6 years ago

Just a heads up - as I saw that the fix got merged after 5.0 was released; if you want to try to get this fix into 5.0.1 you've still got a little time (see http://llvm.org/docs/HowToReleaseLLVM.html#merge-requests for the procedure on how to request it to be merged), but 5.0.1 is not far away.

llvmbot commented 7 years ago

https://reviews.llvm.org/D33852

llvmbot commented 7 years ago

I think I fixed that.

llvmbot commented 7 years ago

We support that, when targeting Windows. In fact, if you pass -target x86_64-pc-win32 you get

[davide@cupiditate decl]$ ../clang++ 1.cpp -o - -c -fms-extensions -target x86_64-pc- win32 -emit-llvm -S |grep Tinky

$"\01?TinkyWinky@@3HA" = comdat any @"\01?TinkyWinky@@3HA" = weak_odr global i32 0, comdat, align 4

While on Windows:

[davide@cupidatate decl]$ ../clang++ 1.cpp -o - -c -fms-extensions -target x86_64- unknown-linux -emit-llvm -S |grep Tinky

1.cpp:1:12: warning: declspec attribute 'selectany' is not supported [-Wignored-attributes] declspec(selectany) int TinkyWinky = false; ^ 1 warning generated. @​TinkyWinky = global i32 0, align 4

I'm not entirely sure why this used to work, it could've been by accident (originally enabled anywhere, then disabled on ELF targets, e.g.).

Instead of keep trying to guess, I'd bisect in order to understand the first commit where we don't mark the GV as comdat. My devstation is going to be fairly busy for the next couple of hours, as I'm building something else, but I'll start a bisection after that.

Thanks,

-- Davide

llvmbot commented 7 years ago

This is my suspect: https://reviews.llvm.org/D32083

llvmbot commented 7 years ago

Did we eve support selectany?

prazek@0x01:~ $ clang++-3.8 --version clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final) Target: x86_64-pc-linux-gnu Thread model: posix InstalledDir: /usr/bin

prazek@0x01:~ $clang++-3.8 1.cpp -fms-extensions -c prazek@0x01:~ $ clang++-3.8 1.cpp -fms-extensions -S -emit-llvm prazek@0x01:~ $ cat 1.ll ; ModuleID = '1.cpp' target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128" target triple = "x86_64-pc-linux-gnu"

$TinkyWinky = comdat any

@​TinkyWinky = weak_odr global i32 0, comdat, align 4

!llvm.ident = !{#0}

!​0 = !{!"clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final)"}

llvmbot commented 7 years ago

And if you dump the IR for any of the two files, at -O3, you see:

@​_ZN7BVUnitTIjE10ShiftValueE = local_unnamed_addr constant i32 5, align 4 @​_ZN7BVUnitTImE10ShiftValueE = local_unnamed_addr constant i32 6, align 4

or similarly, at -O0:

@​_ZN7BVUnitTIjE10ShiftValueE = constant i32 5, align 4 @​_ZN7BVUnitTImE10ShiftValueE = constant i32 6, align 4

(so, no comdat ;)

llvmbot commented 7 years ago

If you have:

$ clang++ --version clang version 5.0.0 (trunk 304592) (llvm/trunk 304594)

$ cat 1.cpp __declspec(selectany) int TinkyWinky = false;

$ clang++ 1.cpp -o 1.o -c -fms-extensions 1.cpp:1:12: warning: declspec attribute 'selectany' is not supported [-Wignored-attributes] declspec(selectany) int TinkyWinky = false; ^

So I don't think this is going to work.

(and if you actually look at the IR emitted by clang, the GV is not marked as comdat http://llvm.org/docs/LangRef.html#comdats, so MC won't emit a COMDAT section and the linker is completely oblivious about the fact it has to discard one of the two as this information is lost in the frontend).

llvmbot commented 7 years ago

If you have:

$ clang++ --version clang version 5.0.0 (trunk 304592) (llvm/trunk 304594)

$ cat 1.cpp __declspec(selectany) int TinkyWinky = false;

$ clang++ 1.cpp -o 1.o -c -fms-extensions 1.cpp:1:12: warning: declspec attribute 'selectany' is not supported [-Wignored-attributes] declspec(selectany) int TinkyWinky = false; ^

So I don't think this is going to work.

(and if you actually look at the IR emitted by clang, the GV is not marked as comdat http://llvm.org/docs/LangRef.html#comdats, so MC won't emit a COMDAT section and the linker is completely oblivious about the fact it has to discard one of the two as this information is lost in the frontend).

llvmbot commented 7 years ago

Yep, it is exactly the one I had: llvm/llvm-project#32525 It was fixed in this patch https://reviews.llvm.org/D33398

Same here. I can reproduce with just these two files:

ld.lld -shared ./out/Release/bin/GCStress/CMakeFiles/GCStress.dir/GCStress.cpp.o ./out/Release/bin/GCStress/CMakeFiles/GCStress.dir/RecyclerTestObject.cpp.o

Now, trying to reduce.

llvmbot commented 7 years ago

Yep, it is exactly the one I had: llvm/llvm-project#32525 It was fixed in this patch https://reviews.llvm.org/D33398

llvmbot commented 7 years ago

Are you trying the most recent trunk? There was a patch today solving crashes in Itanium (mangling of __unaligned), so I hope it is this one, because I had similar problem before it was submited.

llvmbot commented 7 years ago

I think declspec(selectany) should be treated as COMDATs.

I tried to reduce this case but as I build clang with assertions, I found the frontend crashing in mangling. I'll open another bug and take a look at that issue first.

clang-5.0: ../tools/clang/lib/AST/ItaniumMangle.cpp:4518: void {anonymous}::CXXNameMangler::addSubstitution(uintptr_t): Assertion `!Substitutions.count(Ptr) && "Substitution alread$ exists!"' failed.