Page MenuHomePhabricator

[LLD] [COFF] Support ENTRY and SUBSYSTEM in .drectve sections
ClosedPublic

Authored by dmajor on Nov 13 2017, 12:46 PM.

Details

Summary

Adds support for "/ENTRY" and "/SUBSYSTEM" linker options in .drectve sections. Some Mozilla binaries were using these directives and MSVC link.exe appears to allow them. No attempt is made to reconcile these with the options on the command line.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

dmajor created this revision.Nov 13 2017, 12:46 PM
ruiu edited edge metadata.Nov 13 2017, 5:25 PM

Can you add a test?

COFF/Driver.cpp
245

Are you really sure if you need mangle? IIRC, there are some command line options that behave differently in terms of name mangling when they are given via .drctve section, so I wonder if that is not the case for /entry.

mstorsjo added inline comments.
COFF/Driver.cpp
245

Also wrt mangle, it is possible that it also is done differently between gcc or clang in gnu mode, and msvc and clang in msvc mode. But if this is something that only is emitted e.g. with msvc specific pragmas that aren't enabled by default in gnu mode, it might not matter. (It does differ e.g. for mangling of symbol names in dllexport, which is supported in both modes.)

dmajor marked 2 inline comments as done.Nov 20 2017, 1:58 PM
COFF/Driver.cpp
245

Sorry for the slow response. I finally got around to testing an I386 build (which is the only arch where this matters, according to the body of mangle) and I get an undefined symbol if I remove the call to mangle. So it seems that it's necessary.

245

As far as I can tell (based on the comment above PragmaCommentHandler::HandlePragma) this is an msvc specific thing, so I wouldn't expect it to be enabled in gnu mode.

ruiu added a comment.Nov 20 2017, 11:08 PM

I think I can approve if you add a test.

COFF/Driver.cpp
245

Cool! Thank you very much for investigating.

dmajor updated this revision to Diff 124002.Nov 22 2017, 12:34 PM
dmajor marked an inline comment as done.

While writing a test I found a problem: the code that infers /entry should not run if we've previously defined one in a drectve. (In Mozilla's build, it worked by accident because the drectve was redundantly specifying the same function that got inferred.) Updated the patch.

ruiu accepted this revision.Nov 27 2017, 10:55 AM

LGTM

This revision is now accepted and ready to land.Nov 27 2017, 10:55 AM
This revision was automatically updated to reflect the committed changes.