This is an archive of the discontinued LLVM Phabricator instance.

[LLD][COFF] Allow overwriting directives exports with cmd-line exports
ClosedPublic

Authored by aganea on May 1 2023, 12:49 PM.

Details

Summary

MSVC link.exe allows overriding exports on the cmd-line with exports seen in OBJ directives. The typical case is what is described in #62329.

Before this patch, trying to override an export with /export or /def would generate a duplicate warning. This patches tries to replicate the MSVC behavior. A second override on the cmd-line would still generate the warning.

There's still a case which we don't cover: MSVC link.exe is able to demangle an exported OBJ directive function, and match it with a unmangled export function in a .def file. In the meanwhile, one can use the mangled export in the .def to cover that case.

Depends on D149610.

This fixes #62329

Diff Detail

Event Timeline

aganea created this revision.May 1 2023, 12:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 1 2023, 12:49 PM
aganea requested review of this revision.May 1 2023, 12:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 1 2023, 12:49 PM
mstorsjo accepted this revision.May 2 2023, 1:41 AM

Overall LGTM on this, a couple minor comments on the test.

lld/test/COFF/ordinals-override.test
66

Can you add a comment what the contained directive string is?

67

I guess the debug chunk is irrelevant for the test and could be left out?

111

Symbols like this also probably could be left out here, keeping the testcase yaml more minimal?

(For tests like these, we usually start with handcrafted files - or strip down the compiler generated ones as far as possible.)

This revision is now accepted and ready to land.May 2 2023, 1:41 AM
alvinhochun requested changes to this revision.May 2 2023, 2:17 AM
alvinhochun added inline comments.
lld/COFF/DLL.cpp
562

Can we at least have a more descriptive name for the new argument for baseOrdinal?

601–602
651–652
847

Can you check whether MSVC link.exe allows using export ordinal 0?

lld/test/COFF/export.test
17

This seems to depend on changes to llvm-objdump? There should be a separate parent patch to add this output.

Also this patch despite being marked as depending on D149610 somehow does not include the changes in it.

This revision now requires changes to proceed.May 2 2023, 2:17 AM
alvinhochun added inline comments.May 2 2023, 2:20 AM
lld/test/COFF/export.test
17

Sorry, I missed that this line already exists in the current output of llvm-objdump. The second point still applies though.

aganea updated this revision to Diff 518817.May 2 2023, 11:34 AM
aganea marked 9 inline comments as done.

Address comments.

I'm not quite sure how the parenting works in Phabricator I might be doing things wrong.

lld/COFF/DLL.cpp
562

Good point, I noticed that and forgot to change it.

847

It does not. This links suggests it must start at 1: https://learn.microsoft.com/en-us/cpp/build/reference/export-exports-a-function?view=msvc-170

> link.exe ... /EXPORT:foo,@0
LINK : fatal error LNK1119: invalid ordinal number '0'

You've got the parent/child links right, but the diff in this patch should be versus the parent, not versus the base commit on main, i.e. if you think of each patch as a git commit, this patch would be the second commit, which is a child of the previous commit in the stack.

Yes, what jhenderson said. Also you can see in the pre-merge checks it fails to apply the patch, because what it does is that it first applies the parent patch before applying this patch.

alvinhochun added inline comments.May 4 2023, 8:03 AM
lld/test/COFF/ordinals-override.test
66

This is marked "done" but I don't see the comment. Anyway, decoding this shows /export:baz=?baz@@YAXXZ /DEFAULTLIB:"LIBCMT" /DEFAULTLIB:"OLDNAMES" /EXPORT:?foo@@YAXXZ /EXPORT:?bar@@YAXXZ, which seems to be why the pre-merge checks failed on Debian, so I guess they should be cleaned up a bit.

lld-link: error: could not open 'LIBCMT.lib': No such file or directory
lld-link: error: could not open 'OLDNAMES.lib': No such file or directory
aganea updated this revision to Diff 519679.May 4 2023, 3:35 PM
aganea marked an inline comment as done.

Re-apply changes & rebase.

lld/test/COFF/ordinals-override.test
66

Sorry about that, I lost some changes between my rebases. I added them back, please see the new revision. I fixed the linker errors as well.

LGTM except for the issue with the test. I guess wait a few days to see if anyone else have other comments, or someone else can accept too.

lld/test/COFF/ordinals-override.test
66

The pre-merge check is still failing. Can you remove the /DEFAULTLIB: flags from .drectve?

mstorsjo added inline comments.May 5 2023, 3:31 AM
lld/test/COFF/ordinals-override.test
66

Relatedly, it would probably be good if the lld tests would run with some flag that tells it to not look at the surrounding environment, to avoid accidentally making tests that refer to libraries available outside of the tests? Or perhaps it's not too much of an issue as it'd be hit immediately by all buildbots anyway, and the premerge CI in this case too.

aganea updated this revision to Diff 519870.May 5 2023, 8:16 AM

Simplify .drectve section and fix tests for real.

alvinhochun accepted this revision.May 9 2023, 8:45 AM

Since there have been no other comments, I'll go ahead and mark this accepted.

This revision is now accepted and ready to land.May 9 2023, 8:45 AM

Thank you for reviewing @alvinhochun @mstorsjo !