[clang] [MinGW] Explicitly always pass the -fno-use-init-array

[mstorsjo]

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 #55938.

[llvmbot]

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-platform-windows

@llvm/pr-subscribers-clang-driver

Changes

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 #55938.

---

Full diff: https://github.com/llvm/llvm-project/pull/68571.diff

2 Files Affected:

• (modified) clang/lib/Driver/ToolChains/MinGW.cpp (+2)
• (modified) clang/test/Driver/mingw.cpp (+3)

diff --git a/clang/lib/Driver/ToolChains/MinGW.cpp b/clang/lib/Driver/ToolChains/MinGW.cpp
index 5872e13bda358f8..d3d829a8ddbdb95 100644
--- a/clang/lib/Driver/ToolChains/MinGW.cpp
+++ b/clang/lib/Driver/ToolChains/MinGW.cpp
@@ -709,6 +709,8 @@ void toolchains::MinGW::addClangTargetOptions(
     }
   }
 
+  CC1Args.push_back("-fno-use-init-array");
+
   for (auto Opt : {options::OPT_mthreads, options::OPT_mwindows,
                    options::OPT_mconsole, options::OPT_mdll}) {
     if (Arg *A = DriverArgs.getLastArgNoClaim(Opt))
diff --git a/clang/test/Driver/mingw.cpp b/clang/test/Driver/mingw.cpp
index 0f276820d0fff22..bb22a0652b486e1 100644
--- a/clang/test/Driver/mingw.cpp
+++ b/clang/test/Driver/mingw.cpp
@@ -77,3 +77,6 @@
 // CHECK_NO_SUBSYS-NOT: "--subsystem"
 // CHECK_SUBSYS_CONSOLE: "--subsystem" "console"
 // CHECK_SUBSYS_WINDOWS: "--subsystem" "windows"
+
+// RUN: %clang -target i686-windows-gnu -### %s 2>&1 | FileCheck -check-prefix=CHECK_NO_INIT_ARRAY %s
+// CHECK_NO_INIT_ARRAY: "-fno-use-init-array"

[mstorsjo]

This is an alternative to #68570. This has the upside that the UseInitArray flag is set to the correct value throughout the chain, for any other potential users of the field, in case it would affect code generation in other places. The downside is that if we only go with this, then any other language frontend, which might not know about the details about .ctors, will end up with the same bug unless they also add a similar fix. (We could of course go with both fixes as well.)

[efriedma-quic]

We could go with a clang fix, but also make the backend report_fatal_error if you try to use llvm.ctors with UseInitArray on mingw. That keeps everything consistent while also making sure non-clang frontends don't miscompile.

[mstorsjo]

We could go with a clang fix, but also make the backend report_fatal_error if you try to use llvm.ctors with UseInitArray on mingw. That keeps everything consistent while also making sure non-clang frontends don't miscompile.

Hmm, interesting proposition... But wouldn't that essentially break every single invocation of llc with a mingw triple, unless they also explicitly pass the -use-ctors option? And for any other language frontend, I also feel that it would, somewhat abruptly, break every single current user of LLVM for mingw targets, unless they update their calling code to explicitly set UseInitArray to false for this target?

[efriedma-quic]

Hmm...

Maybe the backend should have some notion of a default ctors section. So if the frontend doesn't explicitly specify anything, the backend tries to pick the right default.

[mstorsjo]

Hmm...

Maybe the backend should have some notion of a default ctors section. So if the frontend doesn't explicitly specify anything, the backend tries to pick the right default.

Yep, this is kinda the same issue for lots of target specific options that are set as bools..., while the ideal I guess either would be an optional<bool>, or to initialize the bools to the target specific actual value.

In this case, I would maybe suggest merging both this and #68570 - that should both fix behaviours and make clang aware of the reality, and leave the root design issue for someone else to take on... WDYT?

[efriedma-quic]

Doesn't UseInitArray default to false in LLVM?

Anyway, we already have ways to make booleans optional; it's just a matter of implementing a "default" state, which we already do for certain options like code models. I don't think there's any fundamental architectural work involved in doing that. (See various enums in TargetOptions.h)

[mstorsjo]

Doesn't UseInitArray default to false in LLVM?

The class TargetOptions itself initializes it to false indeed. But codegen::InitTargetOptionsFromCodeGenFlags (which seems to be what e.g. llc uses) explicitly sets it to !getUseCtors(), which only checks the state of the -use-ctors option. And Clang sets it unconditionally based on the codegen option with the same name, which defaults to true and is controlled by the -f[no-]use-init-array option.

Anyway, we already have ways to make booleans optional; it's just a matter of implementing a "default" state, which we already do for certain options like code models. I don't think there's any fundamental architectural work involved in doing that. (See various enums in TargetOptions.h)

Right, so adding an enum DefaultableBool { Default, False, True } and changing the field to that value? That'd be an API break for all users of the struct, but which should be allowed for us to do? All users of TargetOptions would need to change from passing regular bools to passing values from this enum - and at the same time, they get reminded that they can keep it set to Default in case they don't really have a strong opinion on the matter. Is that what you had in mind? Or is there a way to make the migration smoother for users somehow?

It sounds like a somewhat disruptive change for downstreams; I know we're not supposed to compromise upstream design too much around downstream concerns, but it feels somewhat like making more of a fuss than I wanted...

[efriedma-quic]

It's only breaking for downstreams that were messing with the option in the first place. Searching GitHub, the only downstream I can find that actually touches it is rustc.

I understand it's simpler to just override the behavior for MinGW, but I think it's really confusing to have an option in TargetOptions that's arbitrarily ignored by some targets (both for developers reading the code, and for users).

[efriedma-quic]

Whatever we decide to do on the LLVM side, this seems fine for the clang side. It looks like clang uses the value of UseInitArray for some ObjC stuff, in addition to passing it to the backend, so we need the right value in clang in any case.

[mstorsjo]

Right, so adding an enum DefaultableBool { Default, False, True } and changing the field to that value?

With such a definition, I wonder how to interpret it for MSVC targets; UseInitArray would be false, as MSVC uses the .CRT sections, neither .ctors not .init_array. But it shouldn’t be reversed, and false currently means reversing it.

[efriedma-quic]

On targets that don't use the Unix .ctors/.init_array convention, the option doesn't make any sense, so we should just ignore it. For example, the AIX target overrides emitXXStructorList.

[mstorsjo]

Whatever we decide to do on the LLVM side, this seems fine for the clang side.

Yes, this bit should be fine in any case.

It looks like clang uses the value of UseInitArray for some ObjC stuff, in addition to passing it to the backend, so we need the right value in clang in any case.

Indeed, having the correct picture is good, if there's logic that depends on it. For the ObjC case, the code actually looks like this:

    if (CGM.getTriple().isOSBinFormatCOFF())
        InitVar->setSection(".CRT$XCLz");
    else
    {
      if (CGM.getCodeGenOpts().UseInitArray)
        InitVar->setSection(".init_array");
      else
        InitVar->setSection(".ctors");
    }

So in that sense, it doesn't really matter here. (The .CRT$... sections do work on mingw too, even if .ctors normally is used.)