This is an archive of the discontinued LLVM Phabricator instance.

[COFF] Fix constant sharing regression for MinGW
ClosedPublic

Authored by mstorsjo on Jun 25 2018, 2:08 PM.

Details

Summary

This fixes a regression since SVN r334523, where the object files built targeting MinGW were rejected by GNU binutils tools. Prior to that commit, we only put constants in comdat for MSVC configurations.

Diff Detail

Repository
rL LLVM

Event Timeline

mstorsjo created this revision.Jun 25 2018, 2:08 PM
rnk added inline comments.Jun 25 2018, 2:39 PM
include/llvm/CodeGen/TargetLoweringObjectFileImpl.h
137 ↗(On Diff #152770)

I'd prefer it if we didn't put this here, but instead put something in MCAsmInfo, like the existing HasCOFFAssociativeComdats.

lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1354–1356 ↗(On Diff #152770)

It'd be nicer to spell this as a positive check for a gnu or cygwin environment.

mstorsjo added inline comments.Jun 25 2018, 3:06 PM
include/llvm/CodeGen/TargetLoweringObjectFileImpl.h
137 ↗(On Diff #152770)

Ok, what'd be a sensible name for it - HasCOFFComdatConstantSections?

lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1354–1356 ↗(On Diff #152770)

Yes, it probably would. I matched the condition used in the other function moved in the commit that introduced the issue, but I can change it.

LGTM with Reid's suggested changes.

I've pinned down the GNU bfd incompatibility to this:

$ cat const.s
        .section        .rdata,"dr",discard,val
.LCPI0_0:
        .byte 0
$ clang -target x86_64-w64-mingw32 -c const.s -o const.o -c
$ x86_64-w64-mingw32-objdump -d const.o  

const.o:     file format pe-x86-64

x86_64-w64-mingw32-objdump: const.o: Unrecognized storage class 0 for .rdata symbol `val'
x86_64-w64-mingw32-objdump: failed to read symbol table from: const.o
x86_64-w64-mingw32-objdump: error message was: File in wrong format

If I make the symbol val a proper symbol, either by adding a val: in the file, or a .globl val (changing the symbol table entry value from IMAGE_SYM_CLASS_NULL to something else).

I think this maybe even could use the existing flag HasCOFFAssociativeComdats instead of inventing a new one, since it seems to be pretty closely related to the same concept.

mstorsjo updated this revision to Diff 152850.Jun 26 2018, 1:40 AM

Updated to use a flag from MCAsmInfo instead of storing and checking the triplet.

As @rnk doesn't seem to be active at the moment to give this the extra ack (and while this is pretty much what he suggested in the earlier review), I'll go ahead and commit this, since it fixes a regression.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 28 2018, 1:33 PM
This revision was automatically updated to reflect the committed changes.