[LLD] [MinGW] Implement --dependent-load-flag option

[mati865]

Implement MSVC's /DEPENDENTLOADFLAG as --dependent-load-flag and forward it to COFF.
I'm not sure about the name as ld.bfd doesn't support it (yet?).

There is no solid need for it yet, but it's being considered: msys2/MINGW-packages#22216 (comment)

[llvmbot]

@llvm/pr-subscribers-lld

Author: Mateusz Mikuła (mati865)

Changes

Implement MSVC's /DEPENDENTLOADFLAG as --dependent-load-flag and forward it to COFF.
I'm not sure about the name as ld.bfd doesn't support it (yet?).

There is no solid need for it yet, but it's being considered: msys2/MINGW-packages#22216 (comment)

---

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

3 Files Affected:

• (modified) lld/MinGW/Driver.cpp (+3)
• (modified) lld/MinGW/Options.td (+1)
• (modified) lld/test/MinGW/driver.test (+3)

diff --git a/lld/MinGW/Driver.cpp b/lld/MinGW/Driver.cpp
index 553698d4f537fc..b65840e57755e1 100644
--- a/lld/MinGW/Driver.cpp
+++ b/lld/MinGW/Driver.cpp
@@ -398,6 +398,9 @@ bool link(ArrayRef<const char *> argsArr, llvm::raw_ostream &stdoutOS,
                    OPT_no_allow_multiple_definition, false))
     add("-force:multiple");
 
+  if (auto *a = args.getLastArg(OPT_dependent_load_flag))
+    add("-dependentloadflag:" + StringRef(a->getValue()));
+
   if (auto *a = args.getLastArg(OPT_icf)) {
     StringRef s = a->getValue();
     if (s == "all")
diff --git a/lld/MinGW/Options.td b/lld/MinGW/Options.td
index ff7e21fa808f39..a1a4a581ed8bc1 100644
--- a/lld/MinGW/Options.td
+++ b/lld/MinGW/Options.td
@@ -61,6 +61,7 @@ defm demangle: B<"demangle",
     "Do not demangle symbol names">;
 def disable_auto_import: F<"disable-auto-import">,
     HelpText<"Don't automatically import data symbols from other DLLs without dllimport">;
+defm dependent_load_flag: EEq<"dependent-load-flag", "Override default LibraryLoad flags">;
 def disable_runtime_pseudo_reloc: F<"disable-runtime-pseudo-reloc">,
     HelpText<"Don't do automatic imports that require runtime fixups">;
 def disable_stdcall_fixup: F<"disable-stdcall-fixup">,
diff --git a/lld/test/MinGW/driver.test b/lld/test/MinGW/driver.test
index 2831d155fef128..c2aceebde7432e 100644
--- a/lld/test/MinGW/driver.test
+++ b/lld/test/MinGW/driver.test
@@ -455,3 +455,6 @@ RUN: ld.lld -### foo.o -m i386pep --rpath foo 2>&1 | FileCheck -check-prefix=WAR
 RUN: ld.lld -### foo.o -m i386pep -rpath=foo 2>&1 | FileCheck -check-prefix=WARN_RPATH %s
 RUN: ld.lld -### foo.o -m i386pep --rpath=foo 2>&1 | FileCheck -check-prefix=WARN_RPATH %s
 WARN_RPATH: warning: parameter -{{-?}}rpath has no effect on PE/COFF targets
+
+RUN: ld.lld -### foo.o -m i386pe --dependent-load-flag=0x800 2>&1 | FileCheck -check-prefix=DEPENDENT_LOAD_FLAG %s
+DEPENDENT_LOAD_FLAG: -dependentloadflag:0x800

[mati865]

Cc @mstorsjo

[mstorsjo]

This looks reasonable, thanks! Let's hope that binutils can align on the name and behaviour of the flag, if they also get to implementing this.

And as high level view of what it does - this sets one value in the _load_config_used struct, if such a symbol happens to be found at link time, right?

[mati865]

And as high level view of what it does - this sets one value in the _load_config_used struct, if such a symbol happens to be found at link time, right?

Yeah, that's how I understand it. Which means it has an effect only when using Control Flow Guard is enabled.

[mstorsjo]

And as high level view of what it does - this sets one value in the _load_config_used struct, if such a symbol happens to be found at link time, right?

Yeah, that's how I understand it. Which means it has an effect only when using Control Flow Guard is enabled.

Hmm, but isn't _load_config_used linked in whenever it is found in any of the static libraries? https://github.com/llvm/llvm-project/blob/main/lld/COFF/Driver.cpp#L2582-L2584

So with that, _load_config_used should mostly always be linked in, and this option should always end up setting the right flags in the load config, or am I missing something? (CC @alvinhochun)

[mati865]

IIUC static libraries won't even contain that symbol, and in this case it should be provided only by mingw-w64.

I think this example backs up my point:

$ rg "_load_config_used" /clang64/lib -la -g '*.a'
H:/msys64/clang64/lib\liblldCOFF.a
H:/msys64/clang64/lib\libmingwex.a

$ nm /clang64/lib/libmingwex.a | rg _load_config_used
00000000 R _load_config_used
00000138 r _load_config_used__end

$ nm /clang64/lib/liblldCOFF.a | rg _load_config_used
# nothing

Only two static libraries from my whole installation of CLANG64 env in MSYS2 contain _load_config_used string. In case of libmingwex.a it's obviously CRT and liblldCOFF.a got matched only because this string is hardcoded within the linker logic.
FYI: MSYS2 CLANG* envs have Control Flow Guard enabled.

[alvinhochun]

And as high level view of what it does - this sets one value in the _load_config_used struct, if such a symbol happens to be found at link time, right?

Yeah, that's how I understand it. Which means it has an effect only when using Control Flow Guard is enabled.

Hmm, but isn't _load_config_used linked in whenever it is found in any of the static libraries? https://github.com/llvm/llvm-project/blob/main/lld/COFF/Driver.cpp#L2582-L2584

So with that, _load_config_used should mostly always be linked in, and this option should always end up setting the right flags in the load config, or am I missing something? (CC @alvinhochun)

For mingw-w64, at least for now _load_config_used is only compiled when CFG is enabled for mingw-w64-crt itself, because CFG had been the only feature that required it: https://github.com/mingw-w64/mingw-w64/blob/fab7cfe34e5388c920230d0237e3663804d91458/mingw-w64-crt/Makefile.am#L967-L971
One of the benefits is to allow LLD to emit a warning if the user tries to enable CFG with a mingw-w64-crt that was not compiled with CFG. (365d0a5)

But if there is a new reason to require the load config directory, then we need to change how _load_config_used is defined in mingw-w64-crt. What probably should happen is that we move mingw_cfguard_loadcfg.S out of crguard to always compile it, and change its contents to only define the CFG-related symbols when mingw-w64-crt is being built with CFG enabled. This way the CFG stuff won't interfere with non-CFG builds and the LLD warnings can still work.

It may also be beneficial to add a check in LLD to emit a warning if --dependent-load-flag is set but _load_config_used is not available, especially if this feature affects security.

[mstorsjo]

IIUC static libraries won't even contain that symbol, and in this case it should be provided only by mingw-w64.

I think this example backs up my point:

$ rg "_load_config_used" /clang64/lib -la -g '*.a'
H:/msys64/clang64/lib\liblldCOFF.a
H:/msys64/clang64/lib\libmingwex.a

$ nm /clang64/lib/libmingwex.a | rg _load_config_used
00000000 R _load_config_used
00000138 r _load_config_used__end

I don't see how that contradicts what I'm saying :-) As long as libmingwex.a is involved in the link, LLD will pull the _load_config_used symbol from there.

Case in point:

$ x86_64-w64-mingw32-clang test/hello.c -o hello.exe
$ llvm-readobj --coff-load-config hello.exe

File: hello.exe
Format: COFF-x86-64
Arch: x86_64
AddressSize: 64bit
LoadConfig [
  Size: 0x138
  TimeDateStamp: 1970-01-01 00:00:00 (0x0)
  MajorVersion: 0x0
  MinorVersion: 0x0
  GlobalFlagsClear: 0x0
  GlobalFlagsSet: 0x0
  CriticalSectionDefaultTimeout: 0x0
  DeCommitFreeBlockThreshold: 0x0
  DeCommitTotalFreeThreshold: 0x0
  LockPrefixTable: 0x0
  MaximumAllocationSize: 0x0
  VirtualMemoryThreshold: 0x0
  ProcessHeapFlags: 0x0
  ProcessAffinityMask: 0x0
  CSDVersion: 0x0
  DependentLoadFlags: 0x0
  EditList: 0x0
  SecurityCookie: 0x0
  SEHandlerTable: 0x0
  SEHandlerCount: 0
  GuardCFCheckFunction: 0x14000AAD0
  GuardCFCheckDispatch: 0x14000AAD8
  GuardCFFunctionTable: 0x0
  GuardCFFunctionCount: 0
  GuardFlags [ (0x100)
    CF_INSTRUMENTED (0x100)
  ]
  GuardAddressTakenIatEntryTable: 0x0
  GuardAddressTakenIatEntryCount: 0
  GuardLongJumpTargetTable: 0x0
  GuardLongJumpTargetCount: 0
  DynamicValueRelocTable: 0x0
  CHPEMetadataPointer: 0x0
  GuardRFFailureRoutine: 0x0
  GuardRFFailureRoutineFunctionPointer: 0x0
  DynamicValueRelocTableOffset: 0x0
  DynamicValueRelocTableSection: 0
  GuardRFVerifyStackPointerFunctionPointer: 0x0
  HotPatchTableOffset: 0x0
  EnclaveConfigurationPointer: 0x0
  VolatileMetadataPointer: 0x0
  GuardEHContinuationTable: 0x0
  GuardEHContinuationCount: 0
]

(This tested with llvm-mingw, but I presume it would behave the same in msys2.)

For mingw-w64, at least for now _load_config_used is only compiled when CFG is enabled for mingw-w64-crt itself, because CFG had been the only feature that required it: https://github.com/mingw-w64/mingw-w64/blob/fab7cfe34e5388c920230d0237e3663804d91458/mingw-w64-crt/Makefile.am#L967-L971 One of the benefits is to allow LLD to emit a warning if the user tries to enable CFG with a mingw-w64-crt that was not compiled with CFG. (365d0a5)

Ah, right, I forgot about that detail. I presume that this is what @mati865 was referring to?

But if there is a new reason to require the load config directory, then we need to change how _load_config_used is defined in mingw-w64-crt. What probably should happen is that we move mingw_cfguard_loadcfg.S out of crguard to always compile it, and change its contents to only define the CFG-related symbols when mingw-w64-crt is being built with CFG enabled. This way the CFG stuff won't interfere with non-CFG builds and the LLD warnings can still work.

Yep, this sounds reasonable to me. IIRC ARM64EC stuff also requires the load config.

It may also be beneficial to add a check in LLD to emit a warning if --dependent-load-flag is set but _load_config_used is not available, especially if this feature affects security.

That sounds like a very good addition here!

[mati865]

I don't see how that contradicts what I'm saying :-) As long as libmingwex.a is involved in the link, LLD will pull the _load_config_used symbol from there.

Due to the late hour, I forgot to write one more thing: it would be useful to distinguish "typical" static libraries from CRT ones.
Because I understood this part:

Hmm, but isn't _load_config_used linked in whenever it is found in any of the static libraries?

As if any static libraries were allowed to provide it.

Ah, right, I forgot about that detail. I presume that this is what @mati865 was referring to?

Yeah, I meant to say the typical static libraries should never contain that symbol, and mingw-w64 libs provide it only when CFG is enabled.

[mstorsjo]

I don't see how that contradicts what I'm saying :-) As long as libmingwex.a is involved in the link, LLD will pull the _load_config_used symbol from there.

Due to the late hour, I forgot to write one more thing: it would be useful to distinguish "typical" static libraries from CRT ones. Because I understood this part:

Hmm, but isn't _load_config_used linked in whenever it is found in any of the static libraries?

As if any static libraries were allowed to provide it.

Well I don't see a reason why they can't be allowed to do it? It's surely a nontypical setup, but I guess it could be possible for an advanced app to provide it themselves if they expect to run with toolchains that don't provide it.

Anyway, this is all theoretical...

Ah, right, I forgot about that detail. I presume that this is what @mati865 was referring to?

Yeah, I meant to say the typical static libraries should never contain that symbol, and mingw-w64 libs provide it only when CFG is enabled.

Right, indeed - I had forgot about this detail. So it is available as long as mingw-w64 was built with CFG enabled, even if the user app isn't built with CFG enabled (which was what I referred to); user apps can use --dependent-load-flag even if they're not using CFG, as long as the toolchain provides _load_config_used (which currently is tied to how mingw-w64 was built, but we can consider changing that).

---

Anyway, to sum things up - this patch is fine and I guess there's no unfinished discussion thread that would block merging it - right?

@alvinhochun's great suggestion about warning if -dependentloadflag: is used without a load config being available, can be done by either of us as a separate later step.

[alvinhochun]

As if any static libraries were allowed to provide it.

Well I don't see a reason why they can't be allowed to do it? It's surely a nontypical setup, but I guess it could be possible for an advanced app to provide it themselves if they expect to run with toolchains that don't provide it.

Users can supply their own _load_config_used symbol. Even when using MSVC it is sometimes (rarely) done to customize certain fields in the load config dir. For mingw-w64 users, if they urgently need to have DependentLoadFlag set they can build their own loadcfg.o and link it. Though it would be a bad move for an unrelated 3rd-party library to supply it, because it would override the default _load_config_used from the CRT (if there is one) without the user asking for it.

A user-supplied _load_config_used may get outdated and miss fields, so it may be better to change the fields afterwards using some PE editing tools instead. But I digress.

Anyway, to sum things up - this patch is fine and I guess there's no unfinished discussion thread that would block merging it - right?

@alvinhochun's great suggestion about warning if -dependentloadflag: is used without a load config being available, can be done by either of us as a separate later step.

Yeah, LGTM (with one minor inline comment). Though it doesn't seem like there is an urgent need for it to be in main right now, so I feel maybe it is beneficial to wait until a PR to add the warning is ready, so both changes can be merged together. Wdyt?

[alvinhochun]

Probably should exclude the formatting change from the PR.

[mstorsjo]

I pushed an NFC commit to clean this up.

[alvinhochun]

I noticed this change is still here.

[mstorsjo]

I guess it would go away with a rebase, let’s try to do that.

[mstorsjo]

Anyway, to sum things up - this patch is fine and I guess there's no unfinished discussion thread that would block merging it - right?
@alvinhochun's great suggestion about warning if -dependentloadflag: is used without a load config being available, can be done by either of us as a separate later step.

Yeah, LGTM (with one minor inline comment). Though it doesn't seem like there is an urgent need for it to be in main right now, so I feel maybe it is beneficial to wait until a PR to add the warning is ready, so both changes can be merged together. Wdyt?

No strong opinion either way. On the other hand, even if we don't have all the nice convenience in place, it might be nice to get the option merged to align other parties around it (binutils, and users)?

[mati865]

Opened #117400 for the warning.

[mstorsjo]

With #117400 in place, I guess we're good to go to merge this now as well?

[alvinhochun]

Yes, my LGTM with comment still stands. (I see the commit has been marked with some failed checks but they look unrelated to this PR.)