Page MenuHomePhabricator

[lld] [COFF] Add support for aligncomm directives
ClosedPublic

Authored by mstorsjo on Aug 4 2017, 1:58 AM.

Details

Summary

These are emitted for comm symbols in object files, when targeting a GNU environment.

Alternatively, just ignore them since we already align CommonChunk to the natural size of the content (up to 32 bytes). That would only trade away the possibility to overalign small symbols, which doesn't sound like something that might not need to be handled?

Diff Detail

Event Timeline

mstorsjo created this revision.Aug 4 2017, 1:58 AM
ruiu edited edge metadata.Aug 4 2017, 2:13 AM

This patch looks good, but could you give me a pointer to the documentation for this feature?

martell edited edge metadata.Aug 4 2017, 2:14 AM

Rui would probably have to weigh in on what is best here.
I currently disable aligncomm in LLVM for mingw-w64 to work around this with the following patch.
https://github.com/martell/mingw-w64-clang/blob/master/patches/llvm/0002-mingw-w64-use-MSVC-style-ByteAlignment.patch
This does brings about the same problem of needing separate builds of the crt for clang and gcc though so adding support in LLD might make more sense if it is permitted.

In D36304#831621, @ruiu wrote:

This patch looks good, but could you give me a pointer to the documentation for this feature?

Not sure if there's any official documentation on the directive in itself, but here's the binutils docs on the assembler level directive:
https://sourceware.org/binutils/docs-2.23/as/Comm.html

For implementing it, the references I used were MCWinCOFFStreamer::EmitCommonSymbol and the test MC/COFF/comm-align.s in LLVM.

martell accepted this revision.Aug 4 2017, 4:23 AM

Just to follow up. I can confirm I have tested this without applying MSVC byte alignment with my above mentioned patch to the mingw-w64 crt and it works as intended.
I don't think there is any documentation for this feature. This is the original implementation that landed in gcc which might be a good reference.
https://sourceware.org/ml/binutils/2009-05/msg00254.html

This revision is now accepted and ready to land.Aug 4 2017, 4:23 AM

I believe we might have to add support in the def parser for the ALIGNCOMM directive looking at this, though I do not see mingw-w64 use it anywhere.

Rui, do you want to look more on this and the related references, or shall I go ahead and commit it?

ruiu added inline comments.Aug 11 2017, 9:32 AM
COFF/Config.h
142

Please add a comment saying that this is for /aligncomm.

COFF/Driver.cpp
241

Move to the top of this switch as cases are sorted alphabetically.

1177

Since you are using early continue, I'd prefer early continue to handle the following warning too.

auto *DC = dyn_cast<DefinedCommon>(Sym->body());
if (!DC) {
  warn(("/aligncomm symbol " + Name + " of wrong kind");
  continue;
}
1178

Is it possible that getChunk() returns a null?

1179

Throughout the codebase, we store a power of two instead of log2 to Align variables. So please do shift when you parse a value instead of when you use a value.

COFF/DriverUtils.cpp
230

Since the scope is very narrow, use a shorter name for AlignValue. Probably I or V are enough.

mstorsjo updated this revision to Diff 110779.Aug 11 2017, 11:37 AM

Thanks for the review! Updated with the changes that Rui requested.

mstorsjo marked 6 inline comments as done.Aug 11 2017, 11:38 AM
mstorsjo added inline comments.
COFF/Driver.cpp
1178

I'm not sure actually, I did it this way originally just to be extra safe, but I'll remove this if statement for now.

ruiu accepted this revision.Aug 14 2017, 8:47 AM

LGTM

COFF/DriverUtils.cpp
229

Use error() and return from this function instead of calling fatal.

232

Ditto

This revision was automatically updated to reflect the committed changes.
mstorsjo marked an inline comment as done.