Page MenuHomePhabricator

Enable __declspec(selectany) on linux
ClosedPublic

Authored by Prazek on Jun 2 2017, 2:58 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

Prazek created this revision.Jun 2 2017, 2:58 PM
Prazek retitled this revision from Enable __declspec(selectany) on linux) to Enable __declspec(selectany) on linux.Jun 2 2017, 3:06 PM
davide edited edge metadata.Jun 2 2017, 3:11 PM

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 added a comment.Jun 2 2017, 3:15 PM

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 added a comment.Jun 2 2017, 4:40 PM

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

Prazek added a comment.Jun 3 2017, 2:41 PM

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 added a subscriber: martell.EditedJun 3 2017, 3:30 PM

@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 added a comment.Jun 3 2017, 4:32 PM

@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 added a comment.EditedJun 3 2017, 4:59 PM

! 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 updated this revision to Diff 101352.Jun 4 2017, 6:31 AM
  • Fixes
Prazek marked an inline comment as done.Jun 4 2017, 6:33 AM
majnemer added inline comments.
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 added inline comments.Jun 5 2017, 9:25 AM
include/clang/Basic/Attr.td
2421 ↗(On Diff #101352)

Should we allow other OSes than Win32 and Linux?

davide added inline comments.Jun 10 2017, 8:47 PM
include/clang/Basic/Attr.td
2421 ↗(On Diff #101352)

I guess everything ELF should be allowed.

rnk added inline comments.Jun 12 2017, 12:36 PM
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 added inline comments.Jun 13 2017, 4:14 AM
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 added inline comments.Jun 16 2017, 2:09 PM
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 added inline comments.Jun 17 2017, 4:23 PM
include/clang/Basic/Attr.td
2421 ↗(On Diff #101352)

I agree with @rnk here.

Prazek added inline comments.Jul 19 2017, 9:02 PM
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 added inline comments.Jul 20 2017, 9:41 AM
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 marked 9 inline comments as done.Aug 26 2017, 11:17 PM

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

Prazek updated this revision to Diff 112804.Aug 26 2017, 11:17 PM

Enable it on every platform

Prazek updated this revision to Diff 112805.Aug 26 2017, 11:19 PM

remove empty line

Harbormaster completed remote builds in B9663: Diff 112805.

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 added a comment.EditedAug 28 2017, 9:35 AM

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 edited edge metadata.Aug 28 2017, 9:57 AM

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 added a subscriber: hans.Aug 28 2017, 1:53 PM

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.

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 updated this revision to Diff 113189.Aug 29 2017, 7:15 PM
  • Add documentation
aaron.ballman added inline comments.Aug 30 2017, 4:44 AM
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 added inline comments.Aug 30 2017, 10:55 AM
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 added inline comments.Aug 30 2017, 11:06 AM
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 updated this revision to Diff 113357.Aug 30 2017, 10:57 PM
  • docs fixes
Prazek updated this revision to Diff 113359.Aug 30 2017, 11:01 PM
  • docs fixes
aaron.ballman added inline comments.Aug 31 2017, 12:13 PM
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 updated this revision to Diff 113507.Aug 31 2017, 10:12 PM
Prazek marked 4 inline comments as done.
  • 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 accepted this revision.Sep 1 2017, 10:53 AM

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

This revision is now accepted and ready to land.Sep 1 2017, 10:53 AM
Prazek marked an inline comment as done.Sep 5 2017, 2:21 PM
Prazek added inline comments.
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 added inline comments.Sep 8 2017, 6:40 AM
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 added inline comments.Sep 8 2017, 9:10 AM
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 added inline comments.Sep 8 2017, 9:12 AM
include/clang/Basic/AttrDocs.td
3154 ↗(On Diff #113359)

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

aaron.ballman added inline comments.Sep 8 2017, 11:43 AM
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 updated this revision to Diff 115243.Sep 14 2017, 10:32 AM
Prazek marked 8 inline comments as done.

Fixed links

This revision was automatically updated to reflect the committed changes.
Prazek marked 3 inline comments as done.Sep 14 2017, 10:35 AM

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