This is an archive of the discontinued LLVM Phabricator instance.

[LLD] [MinGW] Add support for --dynamicbase, ignore --nxcompat, --tsaware and --high-entropy-va
ClosedPublic

Authored by mstorsjo on Nov 14 2017, 3:38 AM.

Details

Summary

All of these are disabled by default in GNU ld, but enabled by default in lld.

Disable dynamicbase by default since it potentially could cause compatibility issues, but just ignore the others since the lld default should be fine for most concievable cases.

Diff Detail

Repository
rL LLVM

Event Timeline

mstorsjo created this revision.Nov 14 2017, 3:38 AM
mstorsjo updated this revision to Diff 122835.Nov 14 2017, 6:46 AM
mstorsjo retitled this revision from [LLD] [MinGW] Add support for --dynamicbase, --nxcompat and --tsaware to [LLD] [MinGW] Add support for --dynamicbase, --nxcompat, --tsaware and --high-entropy-va.
mstorsjo edited the summary of this revision. (Show Details)

Added --high-entropy-va as well.

mstorsjo updated this revision to Diff 122836.Nov 14 2017, 6:53 AM

Updated the options table and the tests to allow these options with a single dash as well.

rnk edited edge metadata.Nov 14 2017, 1:39 PM

Are all of these defaults important for mingw compatibility, or just -dynamicbase?

I really prefer LLD's defaults, which are inherited from MSVC. Users really shouldn't be making new PE images with executable data segments, or legacy pre-terminal server apps. We need to reserve the right to shed that cruft as time goes on. Microsoft accomplishes that by having very defined multi-year product cycles that are expected to have some migration cost. If the clang or gcc driver sets all these flags by default for the user, I don't care that much and I'd be OK with this patch.

In D40017#925224, @rnk wrote:

Are all of these defaults important for mingw compatibility, or just -dynamicbase?

I really prefer LLD's defaults, which are inherited from MSVC. Users really shouldn't be making new PE images with executable data segments, or legacy pre-terminal server apps. We need to reserve the right to shed that cruft as time goes on. Microsoft accomplishes that by having very defined multi-year product cycles that are expected to have some migration cost. If the clang or gcc driver sets all these flags by default for the user, I don't care that much and I'd be OK with this patch.

Yeah, I would actually tend to agree with this - I'd rather have lld's defaults for those that are unlikely to actually have an impact in practice. And the probability that this will be the main issue for a user trying to link their mingw program with lld is pretty low since there's lots of other issues to run into anyway ;-)

Since there's no negative flag to use to disable it though, should we just ignore these options and pass nothing on to the coff driver, or should we just drop the negative defaults? I.e. if the user passed no option to the mingw driver, pass no flag to the coff driver (and implicitly use that default), if the user passed an option, pass a similar option onwards (even though it just reinforces the default)?

rnk added a comment.Nov 14 2017, 1:47 PM

I think I'm in favor of ignoring options that reinforce LLD's defaults and don't have negatives. We can wait until a user files a bug and then add a negative option for them, or maybe add an escaping mechanism to let them pass the negative MSVC-style option.

mstorsjo updated this revision to Diff 122911.Nov 14 2017, 1:56 PM
mstorsjo retitled this revision from [LLD] [MinGW] Add support for --dynamicbase, --nxcompat, --tsaware and --high-entropy-va to [LLD] [MinGW] Add support for --dynamicbase, ignore --nxcompat, --tsaware and --high-entropy-va.
mstorsjo edited the summary of this revision. (Show Details)
rnk accepted this revision.Nov 14 2017, 2:04 PM

lgtm

This revision is now accepted and ready to land.Nov 14 2017, 2:04 PM
ruiu accepted this revision.Nov 14 2017, 3:01 PM

LGTM

MinGW/Driver.cpp
159 ↗(On Diff #122911)

nit: add a blank line so that this code block doesn't look like a series of if ~ else if ~ else if ~ else. Or maybe you could use :?

This revision was automatically updated to reflect the committed changes.