[LLD] [COFF] Print a warning when using /dependentloadflag without load config #117400

mati865 · 2024-11-22T23:16:26Z

As per request in #113814

llvmbot · 2024-11-22T23:17:02Z

@llvm/pr-subscribers-lld-coff
@llvm/pr-subscribers-lld

@llvm/pr-subscribers-platform-windows

Author: Mateusz Mikuła (mati865)

Changes

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

2 Files Affected:

(modified) lld/COFF/Driver.cpp (+4-1)
(modified) lld/test/COFF/dependentflags.test (+2)

diff --git a/lld/COFF/Driver.cpp b/lld/COFF/Driver.cpp
index a0bff69c6302a2..8fe61d322cbec0 100644
--- a/lld/COFF/Driver.cpp
+++ b/lld/COFF/Driver.cpp
@@ -2307,8 +2307,11 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
 
   // Handle /dependentloadflag
   for (auto *arg :
-       args.filtered(OPT_dependentloadflag, OPT_dependentloadflag_opt))
+       args.filtered(OPT_dependentloadflag, OPT_dependentloadflag_opt)) {
     parseDependentLoadFlags(arg);
+    if (!ctx.symtab.findUnderscore("_load_config_used"))
+      warn("_load_config_used not found, /delayloadflag will have no effect");
+  }
 
   if (tar) {
     llvm::TimeTraceScope timeScope("Reproducer: response file");
diff --git a/lld/test/COFF/dependentflags.test b/lld/test/COFF/dependentflags.test
index 3e90511591c75e..620e65ae0ff4f3 100644
--- a/lld/test/COFF/dependentflags.test
+++ b/lld/test/COFF/dependentflags.test
@@ -20,6 +20,7 @@ RUN: not lld-link %S/Inputs/precomp-a.obj %t.ldcfg.obj /out:%t.exe /nodefaultlib
 RUN: not lld-link %S/Inputs/precomp-a.obj %t.ldcfg.obj /out:%t.exe /nodefaultlib /force /dependentloadflag:zz 2>&1 | FileCheck %s --check-prefix FAIL
 RUN: not lld-link %S/Inputs/precomp-a.obj %t.ldcfg.obj /out:%t.exe /nodefaultlib /force /dependentloadflag:0xf0000 2>&1 | FileCheck %s --check-prefix FAIL-RANGE
 
+RUN: lld-link %S/Inputs/precomp-a.obj /out:%t.exe /nodefaultlib /force /dependentloadflag:0x800 2>&1 | FileCheck %s --check-prefix WARN-NOBASE
 
 BASE: DependentLoadFlags: 0x0
 FLAGS-800: DependentLoadFlags: 0x800
@@ -29,3 +30,4 @@ FAIL: lld-link: error: /dependentloadflag: invalid argument: zz
 FAIL-RANGE: lld-link: error: /dependentloadflag: invalid argument: 0xf0000
 FAIL-NOARG: lld-link: error: /dependentloadflag: no argument specified
 
+WARN-NOBASE: lld-link: warning: _load_config_used not found, /delayloadflag will have no effect

alvinhochun

There should also be a test to make sure

alvinhochun · 2024-11-23T09:39:20Z

lld/COFF/Driver.cpp

    parseDependentLoadFlags(arg);
+    if (!ctx.symtab.findUnderscore("_load_config_used"))
+      warn("_load_config_used not found, /delayloadflag will have no effect");
+  }


Is it right to check for the symbol so early? I thought at this point the objects/libraries haven't even been loaded yet so the symbol will always be missing.

I think the check should be added here instead:

llvm-project/lld/COFF/Writer.cpp

Lines 2604 to 2611 in 3a31427

void Writer::prepareLoadConfig() {

Symbol *sym = ctx.symtab.findUnderscore("_load_config_used");

auto *b = cast_if_present<DefinedRegular>(sym);

if (!b) {

if (ctx.config.guardCF != GuardCFLevel::Off)

warn("Control Flow Guard is enabled but '_load_config_used' is missing");

return;

}

It can probably just check for a non-zero value, like in

llvm-project/lld/COFF/Writer.cpp

Lines 2634 to 2635 in 3a31427

if (ctx.config.dependentLoadFlags)

loadConfig->DependentLoadFlags = ctx.config.dependentLoadFlags;

I had started there, but judging by other uses of ctx.symtab I figured the driver might be the right place to put it.

My preference would be to put the check closer to where the field in _load_config_used is being set, and given that Writer::prepareLoadConfig already does other checks on _load_config_used, it just seems to me to be the more logical place to add the check.

(I was also a bit concerned that resolving the symbol earlier than before may subtly change the behaviour and output of LLD, but I guess there isn't really much to break here.)

In addition, the current code would emit one warning per every /dependentloadflag flag specified (when multiple are passed, only the last one would take effect).

Also, I just noticed the typo – the warning message refers to "/delayloadflag" instead of /dependentloadflag.

My preference would be to put the check closer to where the field in _load_config_used is being set, and given that Writer::prepareLoadConfig already does other checks on _load_config_used, it just seems to me to be the more logical place to add the check.

Sure, I don't have a strong option here for either approach.

In addition, the current code would emit one warning per every /dependentloadflag flag specified (when multiple are passed, only the last one would take effect).

Oh, that's right. I'll see what I can do about it.

Also, I just noticed the typo – the warning message refers to "/delayloadflag" instead of /dependentloadflag.

Dang, good catch!

alvinhochun · 2024-11-23T09:50:48Z

lld/test/COFF/dependentflags.test

@@ -29,3 +30,4 @@ FAIL: lld-link: error: /dependentloadflag: invalid argument: zz
 FAIL-RANGE: lld-link: error: /dependentloadflag: invalid argument: 0xf0000
 FAIL-NOARG: lld-link: error: /dependentloadflag: no argument specified

+WARN-NOBASE: lld-link: warning: _load_config_used not found, /delayloadflag will have no effect


Please add a test to check that no warnings are emitted if _load_config_used exists.

And to be really clear, ideally that test should have the _load_config_used symbol in a static library, so it’s not explicitly referenced or pulled in, other than lld doing it implicitly at the end.

Please add a test to check that no warnings are emitted if _load_config_used exists.

Good point, added.

And to be really clear, ideally that test should have the _load_config_used symbol in a static library, so it’s not explicitly referenced or pulled in, other than lld doing it implicitly at the end.

Just to confirm. It'd be enough to turn ldcfg.obj into a static library and directly pass that library to LLD?

Just to confirm. It'd be enough to turn ldcfg.obj into a static library and directly pass that library to LLD?

Yes, that’s right, thanks!

FWIW, I also felt a little skeptic that this would be enough, as we're only scanning the files that are mentioned directly on the command line:

https://github.com/llvm/llvm-project/blob/llvmorg-19.1.4/lld/COFF/Driver.cpp#L2134-L2169

As technically, the _load_config_used struct can be brought in at a later stage as well, here: https://github.com/llvm/llvm-project/blob/llvmorg-19.1.4/lld/COFF/Driver.cpp#L2397-L2448

I tried to adjust the testcase even further, so that we have two files on the command line - an entry point object, and a library. The entry point object references the library, and the object in the library has got an embedded /defaultlib: directive, pointing to another library, which has got the _load_config_used struct. But even then, the warning works as expected - not warning for this. See mstorsjo@lld-dependentloadflag-warn for my attempt at this.

I also tested with real MSVC libraries (where there's usually a somewhat deep nested structure of /defaultlib: directives between msvcrt/ucrt/vcruntime etc), but there, the _load_config_used is also in a toplevel library, and linking a regular hello world, with /dependentloadflag:0, doesn't produce any warning here.

So with that, I think this may cover enough of the practical cases and keeps the code simple. Or what do you think @alvinhochun?

Thanks for testing this!
Should I cherry-pick 1a5c668 to this PR?

Not sure - what does @alvinhochun think? It makes the testcase more complicated, but also closer to the real world scenario.

The test in that commit RUN: lld-link %t.entry.obj /libpath:%t-libdir %t.ldcfg-defaultlib.lib /out:%t.exe /entry:entry /subsystem:console /dependentloadflag:0x801 2>&1 | FileCheck %s --allow-empty --check-prefix NO-WARN does not have /nodefaultlib, would that not pull in the default CRT libs and invalidate the test?

I needed to remove the /nodefaultlib to make it actually do the thing I wanted to test, having %t.ldcfg-defaultlib.lib contain a /defaultlib: directive, that pulls in another library - with /nodefaultlib it would ignore those.

Linking without /nodefaultlib doesn't pull in the default CRT libs automatically, only if the object files tells it to. In this case, %S/Inputs/precomp-a.obj contains such directives, but I switched that to a different object file for the entry, which doesn't reference LIBCMT.LIB and such.

mati865 · 2024-11-23T13:39:07Z

Noticed lld-link: warning: '_load_config_used' is misaligned (expected alignment to be 8 bytes, got 1 instead) warning that was already present. Is it something that should be taken care of?

mstorsjo · 2024-11-23T16:45:13Z

Noticed lld-link: warning: '_load_config_used' is misaligned (expected alignment to be 8 bytes, got 1 instead) warning that was already present. Is it something that should be taken care of?

Good catch, yes I guess that ideally should be fixed.

mati865 · 2024-11-30T11:40:06Z

Good catch, yes I guess that ideally should be fixed.

FWIW I looked at the assembly, and immediately it became obvious that I should leave it to someone who actually knows what's going on in there.

alvinhochun · 2024-12-02T09:06:38Z

Sorry for the late reply.
I'm still hesitant about the positioning of the check. May I suggest @rnk to take a look?

I also want to point out that

llvm-project/lld/COFF/Writer.cpp

Lines 2634 to 2635 in 3a31427

    
           if (ctx.config.dependentLoadFlags) 
        
             loadConfig->DependentLoadFlags = ctx.config.dependentLoadFlags;

only sets the DependentLoadFlags field of _load_config_used if it is set to a non-zero value. This means passing /dependentloadflag:0 is effectively a no-op, so that if the _load_config_used struct already has a non-zero value set in that field, LLD will not clear the flag. I am not sure if this is intended – if it is, then setting it to 0 should probably not give the warning. (CC @nurmukhametov @tru @aganea from #71537)

One more note that is not strictly related to this PR: Before setting the field, there seems to be no checks to make sure the _load_config_used points to a struct that is actually big enough to contain the field. Is it possible that if a user supplies a malformed _load_config_used struct, LLD would end up corrupting something else in the output binary?

lld-link: warning: '_load_config_used' is misaligned (expected alignment to be 8 bytes, got 1 instead)

(It probably just needs a .p2align 3 before the start of the struct.)

mati865 · 2024-12-02T16:24:32Z

Sorry for the late reply.

No problem.

I'm still hesitant about the positioning of the check.

I don't mind either way, I'm going to change it.

I also want to point out that

llvm-project/lld/COFF/Writer.cpp

Lines 2634 to 2635 in 3a31427

if (ctx.config.dependentLoadFlags)

loadConfig->DependentLoadFlags = ctx.config.dependentLoadFlags;

only sets the DependentLoadFlags field of _load_config_used if it is set to a non-zero value.

I think it has nothing to do with this PR unless I missed something obvious. I'm not going to look into it.

One more note that is not strictly related to this PR

Likewise, I'm not going to look into it.

(It probably just needs a .p2align 3 before the start of the struct.)

I have tried that and several other ways I could find, but the warning wouldn't go away.

nurmukhametov · 2024-12-02T19:42:36Z

only sets the DependentLoadFlags field of _load_config_used if it is set to a non-zero value. This means passing /dependentloadflag:0 is effectively a no-op, so that if the _load_config_used struct already has a non-zero value set in that field, LLD will not clear the flag. I am not sure if this is intended – if it is, then setting it to 0 should probably not give the warning. (CC @nurmukhametov @tru @aganea from #71537)

It looks intended to match the behavior of the MSVC linker. See here: https://github.com/nurmukhametov/llvm-project/blob/e72c949c15208ba3dd53a9cebfee02734965a678/lld/test/COFF/deploadflag-cfg-x64.s#L11

github-actions · 2024-12-03T00:15:29Z

✅ With the latest revision this PR passed the C/C++ code formatter.

…ad config

mstorsjo

This LGTM now, and seems to be in line with what @alvinhochun requested. If there's no more comments, I'd merge this soon. Thanks for sticking to it!

alvinhochun

Thanks, LGTM too.

mstorsjo · 2024-12-05T12:24:36Z

@mati865 Can you uncheck the box in your github settings, to keep your account email secret? If I do a squash&merge (the only one allowed here), it sets an anonymized email for the resulting commit, see https://github.com/llvm/llvm-project/blob/main/.github/workflows/email-check.yaml#L34.

mati865 · 2024-12-05T17:12:48Z

@mstorsjo sure, done.

BTW, thanks for reminding me to update my email address...

llvmbot added lld lld:COFF platform:windows labels Nov 22, 2024

mati865 mentioned this pull request Nov 22, 2024

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

Merged

alvinhochun reviewed Nov 23, 2024

View reviewed changes

mati865 force-pushed the dependentloadflag-warning branch 2 times, most recently from 1e6a534 to defcde6 Compare November 23, 2024 13:34

mati865 force-pushed the dependentloadflag-warning branch from defcde6 to 9704ea0 Compare November 23, 2024 21:41

mati865 force-pushed the dependentloadflag-warning branch from 9704ea0 to c69accc Compare December 3, 2024 00:11

[LLD] [COFF] Print a warning when using /dependentloadflag without lo…

5a75227

…ad config

mati865 force-pushed the dependentloadflag-warning branch from c69accc to 5a75227 Compare December 3, 2024 16:09

cjacek mentioned this pull request Dec 3, 2024

[LLD][COFF] Check load config size before setting its DependentLoadFlags #118535

Merged

mstorsjo approved these changes Dec 4, 2024

View reviewed changes

alvinhochun approved these changes Dec 5, 2024

View reviewed changes

mstorsjo added 2 commits December 5, 2024 13:30

Merge branch 'main' into dependentloadflag-warning

ae6c7b2

Fixup conflict resolution

18b89ee

mstorsjo merged commit 8a6f1ab into llvm:main Dec 5, 2024
8 checks passed

mati865 deleted the dependentloadflag-warning branch December 5, 2024 20:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LLD] [COFF] Print a warning when using /dependentloadflag without load config #117400

[LLD] [COFF] Print a warning when using /dependentloadflag without load config #117400

mati865 commented Nov 22, 2024 •

edited

Loading

llvmbot commented Nov 22, 2024 •

edited

Loading

alvinhochun left a comment

alvinhochun Nov 23, 2024

mati865 Nov 23, 2024

alvinhochun Dec 2, 2024

alvinhochun Dec 2, 2024

mati865 Dec 2, 2024

alvinhochun Nov 23, 2024

mstorsjo Nov 23, 2024

mati865 Nov 23, 2024

mstorsjo Nov 23, 2024

mstorsjo Nov 27, 2024 •

edited

Loading

mati865 Nov 30, 2024

mstorsjo Dec 2, 2024

alvinhochun Dec 2, 2024

mstorsjo Dec 2, 2024

mati865 commented Nov 23, 2024

mstorsjo commented Nov 23, 2024

mati865 commented Nov 30, 2024

alvinhochun commented Dec 2, 2024

mati865 commented Dec 2, 2024

nurmukhametov commented Dec 2, 2024

github-actions bot commented Dec 3, 2024 •

edited

Loading

mstorsjo left a comment

alvinhochun left a comment

mstorsjo commented Dec 5, 2024

mati865 commented Dec 5, 2024

	void Writer::prepareLoadConfig() {
	Symbol *sym = ctx.symtab.findUnderscore("_load_config_used");
	auto *b = cast_if_present<DefinedRegular>(sym);
	if (!b) {
	if (ctx.config.guardCF != GuardCFLevel::Off)
	warn("Control Flow Guard is enabled but '_load_config_used' is missing");
	return;
	}

	if (ctx.config.dependentLoadFlags)
	loadConfig->DependentLoadFlags = ctx.config.dependentLoadFlags;

[LLD] [COFF] Print a warning when using /dependentloadflag without load config #117400

[LLD] [COFF] Print a warning when using /dependentloadflag without load config #117400

Conversation

mati865 commented Nov 22, 2024 • edited Loading

llvmbot commented Nov 22, 2024 • edited Loading

alvinhochun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstorsjo Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mati865 commented Nov 23, 2024

mstorsjo commented Nov 23, 2024

mati865 commented Nov 30, 2024

alvinhochun commented Dec 2, 2024

mati865 commented Dec 2, 2024

nurmukhametov commented Dec 2, 2024

github-actions bot commented Dec 3, 2024 • edited Loading

mstorsjo left a comment

Choose a reason for hiding this comment

alvinhochun left a comment

Choose a reason for hiding this comment

mstorsjo commented Dec 5, 2024

mati865 commented Dec 5, 2024

mati865 commented Nov 22, 2024 •

edited

Loading

llvmbot commented Nov 22, 2024 •

edited

Loading

mstorsjo Nov 27, 2024 •

edited

Loading

github-actions bot commented Dec 3, 2024 •

edited

Loading