Enable __declspec(selectany) on linux

[Prazek]

Summary

This feature was disabled probably by mistake in rL300562
This fixes bug https://bugs.llvm.org/show_bug.cgi?id=33285

[davide]

I think lowering __declspec(selectany) to a comdat for GVs on ELF platform is actually reasonable.
I don't know what happens on Mach-O (as far as I can tell they don't have a real notion of COMDAT, they use coalesced symbols, but I'm not an expert of the format so you might want to ask somebody who's familiar with it). I think Reid should sign-off this change anyway.

[davide]

If you take my example, and you pass -target x86_64-pc-win32-macho:

On clang-3.8, TinkyWinky is lowered to a GV with weak_odr linkage:

$ clang++ 1.cpp -o - -emit-llvm -S -fms-extensions -target x86_64-pc-win32-macho
; ModuleID = '1.cpp'
target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-pc-windows-macho"

@TinkyWinky = weak_odr global i32 0, align 4

!llvm.module.flags = !{!0}
!llvm.ident = !{!1}

!0 = !{i32 1, !"PIC Level", i32 2}
!1 = !{!"clang version 3.8.0 (tags/RELEASE_380/final)"}

On clang-5.0 (trunk, release+assert), the compiler just crashes :(

$ ../clang++ 1.cpp -o - -emit-llvm -S -fms-extensions -target x86_64-pc-win32-macho
clang-5.0: ../tools/clang/lib/CodeGen/CodeGenModule.cpp:479: void clang::CodeGen::CodeGenModule::Release(): Assertion `(LangOpts.ShortWChar || llvm::TargetLibraryInfoImpl::getTarget
WCharSize(Target.getTriple()) == Target.getWCharWidth() / 8) && "LLVM wchar_t size out of sync"' failed.
#0 0x000000000206f67a llvm::sys::PrintStackTrace(llvm::raw_ostream&) (/home/davide/work/llvm/build-release/bin/clang-5.0+0x206f67a)
#1 0x000000000206d46e llvm::sys::RunSignalHandlers() (/home/davide/work/llvm/build-release/bin/clang-5.0+0x206d46e)
#2 0x000000000206d5bc SignalHandler(int) (/home/davide/work/llvm/build-release/bin/clang-5.0+0x206d5bc)
#3 0x00007f72dc473c30 __restore_rt (/lib64/libpthread.so.0+0x10c30)
#4 0x00007f72dafdf765 __GI_raise (/lib64/libc.so.6+0x34765)
#5 0x00007f72dafe136a __GI_abort (/lib64/libc.so.6+0x3636a)
#6 0x00007f72dafd7f97 __assert_fail_base (/lib64/libc.so.6+0x2cf97)
#7 0x00007f72dafd8042 (/lib64/libc.so.6+0x2d042)
#8 0x00000000022cab88 clang::CodeGen::CodeGenModule::Release() (/home/davide/work/llvm/build-release/bin/clang-5.0+0x22cab88)
#9 0x00000000028fbc77 (anonymous namespace)::CodeGeneratorImpl::HandleTranslationUnit(clang::ASTContext&) (/home/davide/work/llvm/build-release/bin/clang-5.0+0x28fbc77)
#10 0x00000000028fa965 clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) (/home/davide/work/llvm/build-release/bin/clang-5.0+0x28fa965)
#11 0x0000000002ccdf38 clang::ParseAST(clang::Sema&, bool, bool) (/home/davide/work/llvm/build-release/bin/clang-5.0+0x2ccdf38)
#12 0x00000000028f9bbc clang::CodeGenAction::ExecuteAction() (/home/davide/work/llvm/build-release/bin/clang-5.0+0x28f9bbc)
#13 0x00000000025bae0e clang::FrontendAction::Execute() (/home/davide/work/llvm/build-release/bin/clang-5.0+0x25bae0e)
#14 0x000000000258c746 clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (/home/davide/work/llvm/build-release/bin/clang-5.0+0x258c746)
#15 0x000000000264ac7b clang::ExecuteCompilerInvocation(clang::CompilerInstance*) (/home/davide/work/llvm/build-release/bin/clang-5.0+0x264ac7b)
#16 0x0000000000aff078 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) (/home/davide/work/llvm/build-release/bin/clang-5.0+0xaff078)
#17 0x0000000000a92775 main (/home/davide/work/llvm/build-release/bin/clang-5.0+0xa92775)
#18 0x00007f72dafcb731 __libc_start_main (/lib64/libc.so.6+0x20731)
#19 0x0000000000afb7e9 _start (/home/davide/work/llvm/build-release/bin/clang-5.0+0xafb7e9)
Stack dump:
0.      Program arguments: /home/davide/work/llvm/build-release/bin/clang-5.0 -cc1 -triple x86_64-pc-windows-macho -emit-llvm -disable-free -main-file-name 1.cpp -mrelocation-model
pic -pic-level 2 -mthread-model posix -mdisable-fp-elim -fmath-errno -masm-verbose -mconstructor-aliases -target-cpu x86-64 -dwarf-column-info -debugger-tuning=gdb -coverage-notes-f
ile /home/davide/work/llvm/build-release/bin/decl/-.gcno -resource-dir /home/davide/work/llvm/build-release/lib/clang/5.0.0 -internal-isystem /home/davide/work/llvm/build-release/li
b/clang/5.0.0/include -fdeprecated-macro -fdebug-compilation-dir /home/davide/work/llvm/build-release/bin/decl -ferror-limit 19 -fmessage-length 181 -fms-extensions -fms-compatibili
ty -fms-compatibility-version=18 -std=c++11 -fno-threadsafe-statics -fdelayed-template-parsing -fobjc-runtime=gcc -fcxx-exceptions -fexceptions -fdiagnostics-show-option -fcolor-dia
gnostics -o - -x c++ 1.cpp
1.      <eof> parser at end of file
2.      Per-file LLVM IR generation
clang-5.0: error: unable to execute command: Aborted (core dumped)
clang-5.0: error: clang frontend command failed due to signal (use -v to see invocation)
clang version 5.0.0 (trunk 304592) (llvm/trunk 304594)
Target: x86_64-pc-windows-macho

[Prazek]

OK, I will try to make it work on linux and windows only

[Prazek]

It seems that this is separate issue - we don't generate comdat for MachO. Indeed we should warn on not supported declspec for macho.

[martell]

@Prazek it was not disabled by mistake.
This was a MS extension and it appeared as though the test case should have been for windows.
I updated it and made the extension available to windows only. (MSVC, Cygwin, Mingw etc)
It didn't expect anyone would want a MS extension for a linux target.

I think it is reasonable to have support for this if you want.
Adding @rnk here as he may have some comments.

[davide]

In D33852#772230, @martell wrote:

@Prazek it was not disabled by mistake.
This was a MS extension and it appeared as though the test case should have been for windows.
I updated it and made the extension available to windows only. (MSVC, Cygwin, Mingw etc)
It didn't expect anyone would want a MS extension for a linux target.

This assumption is not quite right. Also GCC, when targeting Linux, provides a -fms-extensions flag.
There's a bunch of software out there that unfortunately has to target ELF and build with -fms-extensions on, e.g. PS4.

[martell]

! In D33852#772290, @davide wrote:
This assumption is not quite right. Also GCC, when targeting Linux, provides a -fms-extensions flag.
There's a bunch of software out there that unfortunately has to target ELF and build with -fms-extensions on, e.g. PS4.

Seems like bad practice but on a second look that is does appear that PS4 and CUDA both use -fms-extensions and rely on it for some things. argh
CUDA does have a note to move off this in future.
Thanks for clarifying davide

This patch does LGTM minus the nit above.
Lowering selectany spelling to -fdeclspec from -fms-extensions should cover all use cases.
The MachO issue would have been present before rL300562 so I don't think that should have to be fixed here.
It could be covered in a follow up differential as this fixes a regression on my part.

test/Sema/attr-selectany.c

3 ↗

(On Diff #101281)

Based on the current changes you can change -fms-extensions to -fdeclspec
The -fms-extensions coverage is covered in attr-selectany.cpp below

[Prazek]

• Fixes

[Prazek]

I fired bug about the crash: https://bugs.llvm.org/show_bug.cgi?id=33300

[majnemer]

include/clang/Basic/Attr.td

2421 ↗

(On Diff #101352)

selectany should work on targets other than "x86", "x86_64", "arm", "thumb", etc. I think it is only necessary to require that it be a COFF or ELF target.

[Prazek]

include/clang/Basic/Attr.td

2421 ↗

(On Diff #101352)

Should we allow other OSes than Win32 and Linux?

[davide]

include/clang/Basic/Attr.td

2421 ↗

(On Diff #101352)

I guess everything ELF should be allowed.

[rnk]

include/clang/Basic/Attr.td

2421 ↗

(On Diff #101352)

Why not use weak_odr / linkonce_odr on MachO? Microsoft builds Office for Mac and I suspect they use __declspec(selectany).

[Prazek]

include/clang/Basic/Attr.td

2421 ↗

(On Diff #101352)

I think this is what would happen right now. The question is - should we warn about using declspec on macho? Beause not using comdat looks like "not supporting" it, but I am not sure about it.

[rnk]

include/clang/Basic/Attr.td

2421 ↗

(On Diff #101352)

I'm pretty sure weak_odr / linkonce_odr with ld64 on macho are the same as having a comdat. LLVM didn't always have comdats, but it's supported inline functions for a very long time. We should support selectany there.

[davide]

include/clang/Basic/Attr.td

2421 ↗

(On Diff #101352)

I agree with @rnk here.

[Prazek]

include/clang/Basic/Attr.td

2421 ↗

(On Diff #101352)

So does it actually mean that we don't have any requirements for declscpec(any)? It can run on every OS and target

[rnk]

include/clang/Basic/Attr.td

2421 ↗

(On Diff #101352)

I guess so. I think __declspec spellings are controlled by -fdeclspec-extensions, which is off by default except on Windows & PS4. If we remove this constraint, __attribute__((selectany)) will become available everywhere. I guess that's OK.

[Prazek]

Sorry for so late fixes, but it would be good to put it in 5.0

[Prazek]

Enable it on every platform

[Prazek]

remove empty line

[aaron.ballman]

In D33852#853410, @Prazek wrote:

Sorry for so late fixes, but it would be good to put it in 5.0

I do not think this should be in 5.0, as I believe we're only accepting regression fixes at this point.

include/clang/Basic/Attr.td

2477 ↗

(On Diff #112805)

Since we're drastically modifying what platforms this is supported on, can you please add documentation for this attribute?

[Prazek]

In D33852#853982, @aaron.ballman wrote:

In D33852#853410, @Prazek wrote:

Sorry for so late fixes, but it would be good to put it in 5.0

I do not think this should be in 5.0, as I believe we're only accepting regression fixes at this point.

This is a regression. __declspec(selectany) works completely fine on linux with 4.0. Without it clang-5.0 will be useless for any major windows project compiled on linux.

edit:
here is a regression: https://reviews.llvm.org/D32083

[aaron.ballman]

In D33852#854176, @Prazek wrote:

In D33852#853982, @aaron.ballman wrote:

In D33852#853410, @Prazek wrote:

Sorry for so late fixes, but it would be good to put it in 5.0

I do not think this should be in 5.0, as I believe we're only accepting regression fixes at this point.

This is a regression. __declspec(selectany) works completely fine on linux with 4.0. Without it clang-5.0 will be useless for any major windows project compiled on linux.

edit:
here is a regression: https://reviews.llvm.org/D32083

This is also adding new functionality that has had zero testing because it removes *all* target-specific checking for the attribute. Under the previous functionality (changed in D32083), this still required some mention of microsoft something (it went from requiring microsoft extensions to be enabled to instead require a Windows target) -- that's been entirely removed from this patch so now you can use this attribute for all target architectures, so it's not purely fixing a regression. Given how late we are in the release cycle, I am uncomfortable with this going in to 5.0, but I'd have no problems letting it bake for a bit and putting it into 5.1 (or 5.0.1, however we're naming bug releases these days).

[hans]

In D33852#854213, @aaron.ballman wrote:

In D33852#854176, @Prazek wrote:

In D33852#853982, @aaron.ballman wrote:

In D33852#853410, @Prazek wrote:

Sorry for so late fixes, but it would be good to put it in 5.0

I do not think this should be in 5.0, as I believe we're only accepting regression fixes at this point.

This is a regression. __declspec(selectany) works completely fine on linux with 4.0. Without it clang-5.0 will be useless for any major windows project compiled on linux.

edit:
here is a regression: https://reviews.llvm.org/D32083

This is also adding new functionality that has had zero testing because it removes *all* target-specific checking for the attribute. Under the previous functionality (changed in D32083), this still required some mention of microsoft something (it went from requiring microsoft extensions to be enabled to instead require a Windows target) -- that's been entirely removed from this patch so now you can use this attribute for all target architectures, so it's not purely fixing a regression. Given how late we are in the release cycle, I am uncomfortable with this going in to 5.0, but I'd have no problems letting it bake for a bit and putting it into 5.1 (or 5.0.1, however we're naming bug releases these days).

I agree. The idea of taking this so late (and it hasn't even landed in trunk yet) makes me nervous. I think 5.0.1 would be more appropriate.

[Prazek]

In D33852#854213, @aaron.ballman wrote:

In D33852#854176, @Prazek wrote:

In D33852#853982, @aaron.ballman wrote:

In D33852#853410, @Prazek wrote:

Sorry for so late fixes, but it would be good to put it in 5.0

I do not think this should be in 5.0, as I believe we're only accepting regression fixes at this point.

This is a regression. __declspec(selectany) works completely fine on linux with 4.0. Without it clang-5.0 will be useless for any major windows project compiled on linux.

edit:
here is a regression: https://reviews.llvm.org/D32083

This is also adding new functionality that has had zero testing because it removes *all* target-specific checking for the attribute. Under the previous functionality (changed in D32083), this still required some mention of microsoft something (it went from requiring microsoft extensions to be enabled to instead require a Windows target) -- that's been entirely removed from this patch so now you can use this attribute for all target architectures, so it's not purely fixing a regression. Given how late we are in the release cycle, I am uncomfortable with this going in to 5.0, but I'd have no problems letting it bake for a bit and putting it into 5.1 (or 5.0.1, however we're naming bug releases these days).

I don't think this is the case - @rnk said that

I guess so. I think declspec spellings are controlled by -fdeclspec-extensions, which is off by default except on Windows & PS4. If we remove this constraint, attribute__((selectany)) will become available everywhere. I guess that's OK.

So we went from requiring ms extension to not requiring ms extension but only working on windows and now
we require -fdeclspec-extensions or -fms-extensions on every platform other than windows and ps4.

[Prazek]

• Add documentation

[aaron.ballman]

include/clang/Basic/Attr.td

2477 ↗

(On Diff #112805)

This documentation isn't correct. Please see the other attributes (and AttrDocs.td) for an example of how to add the documentation.

[Prazek]

include/clang/Basic/Attr.td

2477 ↗

(On Diff #112805)

Yes, sorry for that. I haven't compile it before sending patch. Is the documentation string good at least?

[aaron.ballman]

include/clang/Basic/Attr.td

2477 ↗

(On Diff #112805)

It's a good start, though a bit terse. It would help to describe what's happening, perhaps even with a short code example. Also, if GCC has good documentation for it, you can also include a link to GCC's documentation a la, "for more information, see: <link>".

[Prazek]

• docs fixes

[Prazek]

• docs fixes

[aaron.ballman]

include/clang/Basic/AttrDocs.td

3154 ↗

(On Diff #113359)

I think you need to set the Category as well.

To test this you should run something like (replacing <root> and fixing up path separators as needed):

clang-tblgen -gen-attr-docs -I <root>\llvm\tools\clang\include <root>\llvm\tools\clang\include\clang\Basic\Attr.td -o <root>\llvm\tools\clang\docs\AttributeReference.rst

3155 ↗

(On Diff #113359)

You should format this the same as other documentation in the file.

3155 ↗

(On Diff #113359)

How about: This attribute appertains to a global symbol, causing it to have a weak definition (linkonce), allowing the linker to select any definition.?

It might also be nice to link the linkonce with https://llvm.org/docs/LangRef.html#linkage-types.

3161 ↗

(On Diff #113359)

Keep the newline at the end of the file.

[Prazek]

• docs fixes

include/clang/Basic/AttrDocs.td

3154 ↗

(On Diff #113359)

Thanks for the testing command. Should I do anything in order to check if docs build?
I enabled docs with -DLLVM_BUILD_DOCS=ON, and I have sphinx. Everything builds, but I don't see docs anywhere.

the docs looks like this:
+selectany (gnu::selectany)
+--------------------------
+.. csv-table:: Supported Syntaxes
+ :header: "GNU", "C++11", "__declspec", "Keyword", "Pragma", "Pragma clang attribute"
+
+ "X","X","X","", "", ""
+
+This attribute appertains to a global symbol, causing it to have a weak
+definition (
+.. _`linkonce`: https://llvm.org/docs/LangRef.html#linkage-types
+), allowing the linker to select any definition.
+
+For more information see
+.. gcc documentation: https://gcc.gnu.org/onlinedocs/gcc-7.2.0/gcc/Microsoft-Windows-Variable-Attributes.html
+

[aaron.ballman]

Assuming no sphinx issues with the docs, this LGTM, thank you!

include/clang/Basic/AttrDocs.td

3154 ↗

(On Diff #113359)

Thanks for the testing command. Should I do anything in order to check if docs build?

On Windows, I use make html within the clang\docs directory to generate the actual sphinx docs -- that will tell you if there are sphinx issues. Be sure you do *not* commit the AttributeReference.rst file that it generates, however.

test/SemaCXX/attr-selectany.cpp

3 ↗

(On Diff #113507)

That has to be the strangest target triple I've ever seen. :-D

[Prazek]

include/clang/Basic/AttrDocs.td

3154 ↗

(On Diff #113359)

I tried building docs but it doesn't seem that there is any target like "html". Do you know if there is documentation on how to build docs? I couldn't find any.

[aaron.ballman]

include/clang/Basic/AttrDocs.td

3154 ↗

(On Diff #113359)

I don't know that we have any docs on that, but I believe the command you want to execute is sphinx-build -b html -d _build/doctrees . _build/html (I pulled this from make.bat in the docs directory.)

[majnemer]

include/clang/Basic/AttrDocs.td

3154 ↗

(On Diff #113359)

This is not the first time this has come up. Can you please add a comment to the top of AttrDocs.td which explains how to generate the HTML?

[aaron.ballman]

include/clang/Basic/AttrDocs.td

3154 ↗

(On Diff #113359)

Definitely a good idea, I'll tackle it when I have a moment.

[aaron.ballman]

include/clang/Basic/AttrDocs.td

3154 ↗

(On Diff #113359)

I found the moment, and there are now some docs with r312811.

Btw, you want to use make -f Makefile.sphinx html (from within the docs directory) to generate the docs on non-Windows platforms.

[Prazek]

Fixed links

[Prazek]

Thanks for help. I checked and docs build and looks legit. Sorry that it took so long.

cfe/trunk/include/clang/Basic/Attr.td

 }
 
 def DLLImport : InheritableAttr, TargetSpecificAttr<TargetWindows> {
   let Spellings = [Declspec<"dllimport">, GCC<"dllimport">];
   let Subjects = SubjectList<[Function, Var, CXXRecord, ObjCInterface]>;
   let Documentation = [DLLImportDocs];
 }
 
-def SelectAny : InheritableAttr, TargetSpecificAttr<TargetWindows> {
+def SelectAny : InheritableAttr {
   let Spellings = [Declspec<"selectany">, GCC<"selectany">];
-  let Documentation = [Undocumented];
+  let Documentation = [SelectAnyDocs];
 }
 
 def Thread : Attr {
   let Spellings = [Declspec<"thread">];
   let LangOpts = [MicrosoftExt];
   let Documentation = [ThreadDocs];
   let Subjects = SubjectList<[Var]>;
 }

cfe/trunk/include/clang/Basic/AttrDocs.td

 
 def ObjCSubclassingRestrictedDocs : Documentation {
   let Category = DocCatType;
   let Content = [{
 This attribute can be added to an Objective-C ``@interface`` declaration to
 ensure that this class cannot be subclassed.
   }];
 }
+
+
+def SelectAnyDocs : Documentation {
+  let Category = DocCatType;
+  let Content = [{
+This attribute appertains to a global symbol, causing it to have a weak
+definition (
+`linkonce <https://llvm.org/docs/LangRef.html#linkage-types>`_
+), allowing the linker to select any definition.
+
+For more information see
+`gcc documentation <https://gcc.gnu.org/onlinedocs/gcc-7.2.0/gcc/Microsoft-Windows-Variable-Attributes.html>`_
+or `msvc documentation <https://docs.microsoft.com/pl-pl/cpp/cpp/selectany>`_.
+}];
+}

cfe/trunk/test/Sema/attr-selectany.c

 // RUN: %clang_cc1 -triple x86_64-win32 -fdeclspec -verify %s
 // RUN: %clang_cc1 -triple x86_64-mingw32 -verify %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -verify -fdeclspec %s
+// RUN: %clang_cc1 -triple x86_64-win32-macho -verify -fdeclspec %s
 
 extern __declspec(selectany) const int x1 = 1; // no warning, const means we need extern in C++
 
 // Should we really warn on this?
 extern __declspec(selectany) int x2 = 1; // expected-warning {{'extern' variable has an initializer}}
 
 __declspec(selectany) void foo() { } // expected-error{{'selectany' can only be applied to data items with external linkage}}

cfe/trunk/test/SemaCXX/attr-selectany.cpp

 // RUN: %clang_cc1 -triple x86_64-win32 -fms-compatibility -fms-extensions -fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -fms-compatibility -fms-extensions -fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -triple x86_64-win32-macho -fms-compatibility -fms-extensions -fsyntax-only -verify -std=c++11 %s
+
 // MSVC produces similar diagnostics.
 
 __declspec(selectany) void foo() { } // expected-error{{'selectany' can only be applied to data items with external linkage}}
 
 __declspec(selectany) int x1 = 1;
 
 const __declspec(selectany) int x2 = 2; // expected-error{{'selectany' can only be applied to data items with external linkage}}