Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

initializers of static data members of class templates not instantiated with the class when targeting the MS ABI #48587

Open Quuxplusone opened 3 years ago

Quuxplusone commented 3 years ago
Bugzilla Link PR49618
Status CONFIRMED
Importance P normal
Reported by Zoe Carver (z.zoelec2@gmail.com)
Reported on 2021-03-17 10:54:42 -0700
Last modified on 2021-03-18 12:10:05 -0700
Version trunk
Hardware All All
CC blitzrakete@gmail.com, dgregor@apple.com, erik.pilkington@gmail.com, llvm-bugs@lists.llvm.org, richard-llvm@metafoo.co.uk
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

I think this Godbolt demonstrates the problem pretty clearly: https://godbolt.org/z/Mv5osj

Basically, if there's an invalid decl (such as the var decl shown in the Godbolt), neither the decl nor its parent will be marked invalid on Windows. But, on all other platforms, they will (properly?) be marked as invalid. I ran into this problem while testing Swift's C++ interop; it was causing crashes because we had no way to differentiate valid and invalid decls on Windows, so invalid decls were slipping through.

I thought the MS compatibility features might have been the cause of this issue, but the problem persists when I pass -fno-delayed-template-parsing and -fno-ms-compatibility.

I'd like to understand why this is happening and if it's the intended behavior. I'd also like to find a good short-term solution so that I can stop Swift's C++ interop from crashing. (Whether that's a flag to disable it or another API that I can use to find out if the decl is actually invalid. Right now, I'm using "isInvalidDecl.")

Quuxplusone commented 3 years ago
This looks like inline variable semantics leaking through...

MSVC treats an in-class definition of a static data member as being a
definition. For example,

struct T { static const bool value = true; };
extern const bool &b = T::value;

does not require a separate definition of T::value under the MS ABI. Because
this affects the ABI, Clang implements the same behavior.

However, this means that Clang thinks the initializer of the static data member
in your testcase is part of the definition of the variable, not part of the
declaration, and instantiating a class doesn't instantiate the definition
portions of any static data members (http://eel.is/c++draft/temp.inst#3.1), so
Clang doesn't instantiate the initializer with the class definition and instead
delays the instantiation until the variable is used (like it would for an
inline static data member).

Clang does diagnose if you later mention Invalid::value (more specifically, if
you odr-use it or if it's needed for constant evaluation), because that
triggers the instantiation of the definition.

Given that MSVC instantiates the initializer with the class declaration in this
case, and doing so is more consistent with the language rules, we should
probably do the same.
Quuxplusone commented 3 years ago

Sorry for the delay in responding. Thanks for all that information, it was super helpful. I did some digging and it seems like this stems from Clang incorrectly marking constexpr var decls as implicitly inline on Windows. I think I'll be able to put up a fix for this later today.

Thanks again for the help and quick reply!