Page MenuHomePhabricator

LLVM: Make GNU COFF Aligncomm optional
Needs ReviewPublic

Authored by martell on Aug 6 2017, 8:33 AM.

Details

Reviewers
rnk
mstorsjo
Summary

This is a hacky patch I created to remove aligncomm, which limits us to the 32 bytes.

My real goal however is to make aligncomm a compile time flag.
So that if someone really wants >32 bytes alignment the can enable it with a clang flag.

I am not sure how I would have a flag propagate all the way down to this area of LLVM
I would also like a name suggestion for the flag.

Adding reid here because he is super helpful with these kind of things :)

Why you may ask?
With D36304 we will have support for aligncomm in lld but we will never have support in msvc link.
I would like mingw-w64 to be linkable with link.exe

Note: Achieving this also requires a patch for binutils to rename __image_base__ to __ImageBase for MSVC link which I plan to upstream into mingw-w64 and binutils.

If a flag is not possible from here alternatively we could use MS alignment for anything under 32 bytes and enable aligncomm for anything over that for gnu targets.

Diff Detail

Repository
rL LLVM

Event Timeline

martell created this revision.Aug 6 2017, 8:33 AM
martell added a reviewer: mstorsjo.
martell edited the summary of this revision. (Show Details)
martell edited the summary of this revision. (Show Details)
martell edited the summary of this revision. (Show Details)
martell edited the summary of this revision. (Show Details)Aug 6 2017, 8:35 AM
martell edited the summary of this revision. (Show Details)Aug 6 2017, 8:43 AM
martell edited the summary of this revision. (Show Details)Aug 6 2017, 8:53 AM
martell updated this revision to Diff 109926.Aug 6 2017, 9:25 AM

updated to only use aligncomm for alignments > 32

mstorsjo edited edge metadata.Aug 6 2017, 9:43 AM

I think one important question that needs to be looked into is, does GNU ld align comm symbols at all if we omit this? There's a neat testcase in lld (COFF/common-something iirc) that you might use as base for figuring that out. If it doesn't, I'm afraid we can't do this.

I think one important question that needs to be looked into is, does GNU ld align comm symbols at all if we omit this? There's a neat testcase in lld (COFF/common-something iirc) that you might use as base for figuring that out. If it doesn't, I'm afraid we can't do this.

In the very least we should be able to add a flag like -fno-aligncomm for clang to compile without it. This way we can decided at compile time if we want support for linking with link.exe or gnu ld.

martell updated this revision to Diff 109931.Aug 6 2017, 12:02 PM
martell updated this revision to Diff 109940.Aug 6 2017, 3:09 PM

GCC already has such a flag, it calls it -mpe-aligned-commons and it is on by default. I don't think we should stop emitting aligncomm for GNU targets by default, we shouldn't (and don't AFAIK) emit aligncomm for MSVC targets.

GCC already has such a flag, it calls it -mpe-aligned-commons and it is on by default. I don't think we should stop emitting aligncomm for GNU targets by default, we shouldn't (and don't AFAIK) emit aligncomm for MSVC targets.

Thanks @majnemer, in that case we should also have a flag -mpe-aligned-commons and be able to call -mno-pe-aligned-commons to disable it for mingw at compile time if desired.
The question I have is generally about how we propagate this flag all the way down into llvm in MCWinCOFFStreamer.cpp

GCC already has such a flag, it calls it -mpe-aligned-commons and it is on by default. I don't think we should stop emitting aligncomm for GNU targets by default, we shouldn't (and don't AFAIK) emit aligncomm for MSVC targets.

Thanks @majnemer, in that case we should also have a flag -mpe-aligned-commons and be able to call -mno-pe-aligned-commons to disable it for mingw at compile time if desired.
The question I have is generally about how we propagate this flag all the way down into llvm in MCWinCOFFStreamer.cpp

MCTargetOptions is where this would live. Take a look at how MCIncrementalLinkerCompatible and MCRelaxAll are implemented.

lib/MC/MCWinCOFFStreamer.cpp
231–232

Use parens consistently.