Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MSYS2] Ordered dynamic initialization is not sequenced #55938

Closed
silverqx opened this issue Jun 8, 2022 · 18 comments · Fixed by #68571 or llvm/llvm-project-release-prs#727 · May be fixed by #68570
Closed

[MSYS2] Ordered dynamic initialization is not sequenced #55938

silverqx opened this issue Jun 8, 2022 · 18 comments · Fixed by #68571 or llvm/llvm-project-release-prs#727 · May be fixed by #68570
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' platform:windows

Comments

@silverqx
Copy link

silverqx commented Jun 8, 2022

Following simple code example crashes or doesn't work correctly on MSYS2 UCRT64 clang++ and on MSYS2 CLANG64 clang++. All other compilers don't have problem with this, gcc/clang on linux, msvc or clang-cl msvc and also MSYS2 UCRT64 g++.

It crashes because ConstAppInline::LEFT is uninitialized during the Model::m_fillable initialization.

I think that this is happening, here should apply Ordered dynamic initialization , so the initialization should be sequenced:

within a single translation unit, initialization of these variables is always sequenced in exact order their definitions appear in the source code.

The consequence is that you can not initialize inline static data member with Inline const variables at namespace scope variable with external linkage. ☝️

Click to expand!
#include <iostream>
#include <string>
#include <vector>

//#include <QDebug>
//#include <QStringLiteral>

namespace ConstAppInline
{

   // Common chars
//    inline const QChar DOT = QChar('.');

   // Common strings
   inline const std::string LEFT = "left";
//    inline const QString LEFT = "left";

} // namespace ConstAppInline


class Model
{
   inline static std::vector<std::string> m_fillable {ConstAppInline::LEFT};
//    inline static QStringList m_fillable {ConstAppInline::LEFT};

public:
   inline void run() const
   {
       std::cout << m_fillable.size() << "\n";

       for (const auto &s : m_fillable)
           std::cout << s << "\n";

//        qDebug() << m_fillable.size();

//        qDebug() << m_fillable;
   }
};

int main(int /*unused*/, char */*unused*/[])
{
   Model m;
   m.run();

   return 0;
}

The example contains two types, one types from stl library and other for Qt types, in both versions the ConstAppInline::LEFT is uninitialized.

Qt version crashes and the output with stl types is like following:

1

Of course, it should look like this with stl types:

1
left

And like this with Qt types:

1
QList("left")

This is the long-running bug that I'm experiencing with during the development of one of my libraries and hoped that it will be fixed, but it wasn't. The problem exists as long as I remember, from Clang 10/11.

@llvmbot
Copy link
Collaborator

llvmbot commented Oct 1, 2023

@llvm/issue-subscribers-clang-codegen

Following simple code example crashes or doesn't work correctly on `MSYS2 UCRT64 clang++` and on `MSYS2 CLANG64 clang++`. All other compilers don't have problem with this, gcc/clang on linux, msvc or clang-cl msvc and also MSYS2 UCRT64 g++.

It crashes because ConstAppInline::LEFT is uninitialized during the Model::m_fillable initialization.

I think that this is happening, here should apply Ordered dynamic initialization , so the initialization should be sequenced:

> within a single translation unit, initialization of these variables is always sequenced in exact order their definitions appear in the source code.

The consequence is that you can not initialize inline static data member with Inline const variables at namespace scope variable with external linkage. ☝️

<details>
<summary>Click to expand!</summary>

#include &lt;iostream&gt;
#include &lt;string&gt;
#include &lt;vector&gt;

//#include &lt;QDebug&gt;
//#include &lt;QStringLiteral&gt;

namespace ConstAppInline
{

   // Common chars
//    inline const QChar DOT = QChar('.');

   // Common strings
   inline const std::string LEFT = "left";
//    inline const QString LEFT = "left";

} // namespace ConstAppInline


class Model
{
   inline static std::vector&lt;std::string&gt; m_fillable {ConstAppInline::LEFT};
//    inline static QStringList m_fillable {ConstAppInline::LEFT};

public:
   inline void run() const
   {
       std::cout &lt;&lt; m_fillable.size() &lt;&lt; "\n";

       for (const auto &amp;s : m_fillable)
           std::cout &lt;&lt; s &lt;&lt; "\n";

//        qDebug() &lt;&lt; m_fillable.size();

//        qDebug() &lt;&lt; m_fillable;
   }
};

int main(int /*unused*/, char */*unused*/[])
{
   Model m;
   m.run();

   return 0;
}

</details>

The example contains two types, one types from stl library and other for Qt types, in both versions the ConstAppInline::LEFT is uninitialized.

Qt version crashes and the output with stl types is like following:

1

Of course, it should look like this with stl types:

1
left

And like this with Qt types:

1
QList("left")

This is the long-running bug that I'm experiencing with during the development of one of my libraries and hoped that it will be fixed, but it wasn't. The problem exists as long as I remember, from Clang 10/11.

@alvinhochun
Copy link
Contributor

This looks like to be a mishap with the .ctors section.

For your code snippet https://godbolt.org/z/ccreG5rdK, Clang emits two separate functions to initialize the two non-local variables:

  • __cxx_global_var_init initializes ConstAppInline::LEFT
  • __cxx_global_var_init.1 initializes Model::m_fillable

Pointers to these functions are placed in the .ctorssection in forward order:

        .section        .ctors,"dw",associative,ConstAppInline::LEFT[abi:cxx11]
        .p2align        3, 0x0
        .quad   __cxx_global_var_init
        .section        .ctors,"dw",associative,Model::m_fillable[abi:cxx11]
        .p2align        3, 0x0
        .quad   __cxx_global_var_init.1

However, the trap is that functions in the .ctors list are executed in reverse order. The end result is that the constructors get executed in reverse.

This turns out to not affect Linux because instead of the .ctors section, it uses the .init_array section which has forward order.

GCC for mingw-w64 target is not affected because GCC generates only one function _GLOBAL__sub_I_main to put in the .ctors section. This function in turn calls __static_initialization_and_destruction_0(), which handles the initialization of both ConstAppInline::LEFT and Model::m_fillable in sequence.

Note: I wrote the above entirely based on the output from Compiler Explorer without testing locally, so it is possible I may have got something wrong.

CC @MaskRay @mstorsjo

@alvinhochun
Copy link
Contributor

I was trying to see if this can be reproduced on Linux: https://godbolt.org/z/zW4a8MYE3

I found that the option -fno-use-init-array can be used to tell Clang to use the .ctors section instead of .init_array. However, when I do this, Clang correctly generates the section in reverse order unlike for MinGW target:

        .section        .ctors,"aGw",@progbits,Model::m_fillable[abi:cxx11],comdat
        .p2align        3, 0x90
        .quad   __cxx_global_var_init.1
        .section        .ctors,"aGw",@progbits,ConstAppInline::LEFT[abi:cxx11],comdat
        .p2align        3, 0x90
        .quad   __cxx_global_var_init

(The linked executable ends up producing the wrong output, but that seems to be caused by different reasons, so irrelevant here.)

It turns out CodeGen/AsmPrinter has code to reverse the constructor / destructor list specifically for this purpose, but it's not being activated by the Clang MinGW driver:

// Emit the structors in reverse order if we are using the .ctor/.dtor
// initialization scheme.
if (!TM.Options.UseInitArray)
std::reverse(Structors.begin(), Structors.end());

Passing -fno-use-init-array to cc1 fixes the issue (https://godbolt.org/z/jhnn4r1YT). So the obvious fix for the issue is to add CC1Args.push_back("-fno-use-init-array"); to:

void toolchains::MinGW::addClangTargetOptions(
const llvm::opt::ArgList &DriverArgs, llvm::opt::ArgStringList &CC1Args,
Action::OffloadKind DeviceOffloadKind) const {

mstorsjo added a commit to mstorsjo/llvm-project that referenced this issue Oct 9, 2023
On MinGW targets, the .ctors section is always used (as opposed
to on ELF platforms, where .init_section is the default but
one can select using .ctors, with the -use-ctors option, or the
UseInitArray field in TargetOptions).

Apply the reverse ordering regardless of whether the caller has
set the UseInitArray flag for this target, as this target
unconditionally uses the .ctors section anyway.

For the CodeGen/X86/constructor.ll testcase, note how this now
produces the same output ordering as for ELF targets with -use-ctors.

This fixes llvm#55938.
mstorsjo added a commit to mstorsjo/llvm-project that referenced this issue Oct 9, 2023
On MinGW targets, the .ctors section is always used for constructors.

Make sure that all layers of code generation is aware of this,
wherever it matters, by passing the -fno-use-init-array option,
setting the TargetOptions field UseInitArray to false.

This fixes llvm#55938.
@mstorsjo
Copy link
Member

mstorsjo commented Oct 9, 2023

See #68570 and #68571 for potential fixes for the issue.

@silverqx
Copy link
Author

silverqx commented Oct 9, 2023

I will try to execute my project after it hits stable branch and let know if still crashes, also thx for fixes.

@mstorsjo
Copy link
Member

Reopening for backporting the fix, which should be small and safe enough.

@mstorsjo mstorsjo reopened this Oct 10, 2023
@mstorsjo mstorsjo added this to the LLVM 17.0.X Release milestone Oct 10, 2023
@mstorsjo
Copy link
Member

/cherry-pick a2b8c49

@llvmbot
Copy link
Collaborator

llvmbot commented Oct 10, 2023

/branch llvm/llvm-project-release-prs/issue55938

llvmbot pushed a commit to llvm/llvm-project-release-prs that referenced this issue Oct 10, 2023
On MinGW targets, the .ctors section is always used for constructors.

When using the .ctors section, the constructors need to be emitted in
reverse order to get them execute in the right order. (Constructors with
a specific priority are sorted separately by the linker later.) In LLVM,
in CodeGen/AsmPrinter/AsmPrinter.cpp, there's code that reverses them
before writing them out, executed when using the .ctors section. This
logic is done whenever TM.Options.UseInitArray is set to false. Thus,
make sure to set UseInitArray to false for this target.

This fixes llvm/llvm-project#55938.

(cherry picked from commit a2b8c49c1839076b540c542c024fcfe2361a3e47)
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 10, 2023

/pull-request llvm/llvm-project-release-prs#727

@silverqx
Copy link
Author

I want to ask in which release it will be available?

@mstorsjo
Copy link
Member

I want to ask in which release it will be available?

It's in git main now, so it would be included in 18.x.

But I put in a request to backport the fix to 17.x, so if that's accepted in llvm/llvm-project-release-prs#727 and deemed acceptable by the release managers, it could be in one of the following point releases (e.g. 17.0.3 next week).

@silverqx
Copy link
Author

Ok, thx for info

@owenca owenca closed this as completed Oct 15, 2023
@mstorsjo
Copy link
Member

I wonder why this was closed? It was supposed to be open while waiting for the backport. Any idea what happened @owenca ?

@mstorsjo mstorsjo reopened this Oct 16, 2023
@owenca
Copy link
Contributor

owenca commented Oct 16, 2023

Not sure what happened. My apologies!

tru pushed a commit to llvm/llvm-project-release-prs that referenced this issue Oct 16, 2023
On MinGW targets, the .ctors section is always used for constructors.

When using the .ctors section, the constructors need to be emitted in
reverse order to get them execute in the right order. (Constructors with
a specific priority are sorted separately by the linker later.) In LLVM,
in CodeGen/AsmPrinter/AsmPrinter.cpp, there's code that reverses them
before writing them out, executed when using the .ctors section. This
logic is done whenever TM.Options.UseInitArray is set to false. Thus,
make sure to set UseInitArray to false for this target.

This fixes llvm/llvm-project#55938.

(cherry picked from commit a2b8c49c1839076b540c542c024fcfe2361a3e47)
@mstorsjo
Copy link
Member

I want to ask in which release it will be available?

The fix was backported to 17.0.3, which was released yesterday, and a release of llvm-mingw with that version of LLVM is out now as well: https://github.com/mstorsjo/llvm-mingw/releases/tag/20231017

@silverqx
Copy link
Author

Great, thx

@EugeneZelenko EugeneZelenko added clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' and removed clang:codegen labels Oct 18, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 18, 2023

@llvm/issue-subscribers-clang-driver

Author: Silver Zachara (silverqx)

Following simple code example crashes or doesn't work correctly on `MSYS2 UCRT64 clang++` and on `MSYS2 CLANG64 clang++`. All other compilers don't have problem with this, gcc/clang on linux, msvc or clang-cl msvc and also MSYS2 UCRT64 g++.

It crashes because ConstAppInline::LEFT is uninitialized during the Model::m_fillable initialization.

I think that this is happening, here should apply Ordered dynamic initialization , so the initialization should be sequenced:

> within a single translation unit, initialization of these variables is always sequenced in exact order their definitions appear in the source code.

The consequence is that you can not initialize inline static data member with Inline const variables at namespace scope variable with external linkage. ☝️

<details>
<summary>Click to expand!</summary>

#include &lt;iostream&gt;
#include &lt;string&gt;
#include &lt;vector&gt;

//#include &lt;QDebug&gt;
//#include &lt;QStringLiteral&gt;

namespace ConstAppInline
{

   // Common chars
//    inline const QChar DOT = QChar('.');

   // Common strings
   inline const std::string LEFT = "left";
//    inline const QString LEFT = "left";

} // namespace ConstAppInline


class Model
{
   inline static std::vector&lt;std::string&gt; m_fillable {ConstAppInline::LEFT};
//    inline static QStringList m_fillable {ConstAppInline::LEFT};

public:
   inline void run() const
   {
       std::cout &lt;&lt; m_fillable.size() &lt;&lt; "\n";

       for (const auto &amp;s : m_fillable)
           std::cout &lt;&lt; s &lt;&lt; "\n";

//        qDebug() &lt;&lt; m_fillable.size();

//        qDebug() &lt;&lt; m_fillable;
   }
};

int main(int /*unused*/, char */*unused*/[])
{
   Model m;
   m.run();

   return 0;
}

</details>

The example contains two types, one types from stl library and other for Qt types, in both versions the ConstAppInline::LEFT is uninitialized.

Qt version crashes and the output with stl types is like following:

1

Of course, it should look like this with stl types:

1
left

And like this with Qt types:

1
QList("left")

This is the long-running bug that I'm experiencing with during the development of one of my libraries and hoped that it will be fixed, but it wasn't. The problem exists as long as I remember, from Clang 10/11.

@silverqx
Copy link
Author

silverqx commented May 30, 2024

Now I have tested my project with inline constants enabled and it builds, so this bug is fixed for sure. Thx @mstorsjo @alvinhochun

[OT] But a very similar bug exists for extern constants for shared builds 😅, now I tested it as well and it still exists, but it's reproducible only with clang-cl with msvc, MSYS2 is fine with this extern constants shared build combination.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment