Opened 3 years ago

Last modified 9 months ago

#12965 upstream bug

String merging broken on Windows

Reported by: bgamari Owned by:
Priority: normal Milestone: 8.8.1
Component: Compiler Version: 8.0.1
Keywords: Cc: Phyx-
Operating System: Windows Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D3384
Wiki Page:

Description

#9577 introduced string literal merging via the linker. Unfortunately I haven't been able to get this to function on Windows, resulting in testsuite failures of #9577.

Even gcc appears to do the wrong thing here,

$ cat hi.c
#include <stdio.h>
const char *hi = "hello world";
extern const char *hi2;
void main() { 
    printf("%p %p", hi, hi2);
}
$ cat hi2.c
const char *hi2 = "hello world";
$ gcc -c hi.c; gcc -c hi2.c; gcc -fmerge-all-constants hi2.o hi.o
$ ./a.exe 
0x100403040 0x100403030
$ gcc -s hi2.c
$ cat hi2.s
	.file	"hi2.c"
	.globl	hi
	.section .rdata,"dr"
.LC0:
	.ascii "hello world\0"
	.data
	.align 8
hi:
	.quad	.LC0
	.ident	"GCC: (GNU) 5.3.0"

Change History (12)

comment:1 Changed 3 years ago by Ben Gamari <ben@…>

In be5384ce/ghc:

testsuite: Mark T9577 as broken due to #12965

Test Plan: validate

Reviewers: austin, Phyx

Reviewed By: Phyx

Subscribers: thomie

Differential Revision: https://phabricator.haskell.org/D2835

GHC Trac Issues: #12965

comment:2 Changed 3 years ago by Phyx-

Constant merging was disabled on Windows by a stupid check that was introduced late in GCC6. https://github.com/gcc-mirror/gcc/blob/gcc-6-branch/gcc/configure.ac#L2928

The check essentially ensures that only ELF platform support that flag.

comment:3 Changed 3 years ago by bgamari

Status: newupstream

Do you know when can we expect a fixed GCC release, Phyx?

comment:4 Changed 3 years ago by bgamari

Milestone: 8.2.18.4.1

Bumping off to 8.4 since we really can't do anything about this ourselves anyways.

comment:5 Changed 3 years ago by Phyx-

Milestone: 8.4.18.2.1

I've worked out the changes required for this on GCC, but binutils may need a patch as well. The one for GCC I might be able to get away with as a bugfix, but the binutils one...

If I'm able to fix them then I can ask the mingw-w64 guys for another patch release. So we don't have to wait for a release.

comment:6 Changed 3 years ago by Phyx-

Milestone: 8.2.18.4.1

hmm trac had a mind of it's own.

comment:7 Changed 2 years ago by Phyx-

Differential Rev(s): Phab:D3384

comment:8 Changed 2 years ago by Phyx-

Status: upstreampatch

comment:9 Changed 19 months ago by bgamari

Milestone: 8.4.18.6.1

This ticket won't be resolved in 8.4; remilestoning for 8.6. Do holler if you are affected by this or would otherwise like to work on it.

comment:10 Changed 14 months ago by bgamari

Milestone: 8.6.18.8.1

These won't be fixed for 8.6, bumping to 8.8.

comment:11 Changed 9 months ago by bgamari

Status: patchupstream

Phyx, what is the status of the upstream gcc patches?

comment:12 Changed 9 months ago by Phyx-

The specific implementation in GCC/binutils won't work for non-ELF targets it turns out.

To be more specific, the PE targets of GAS don't have section attributes for merging. e.g. they don't have SHF_MERGE and SHF_STRINGS. So GCC doesn't have anything it can do here. What it does have is comdat support/linkonce support. So it can't do the cross object code pooling. Only the constant pooling within a single object code.

However I have an implementation that should do this using a similar method as the D compiler uses by leveraging ld's .gnu.linkonce support for PE targets.

This method however needs a guarantee from the compiler that it doesn't output duplicate string constants, or GAS will end up merging them and breaking the approach. (This is what was going wrong in the Diff as WIP on Phab).

It seems that GHC doesn't do this at the moment, it only happens to have this effect some of the them due to CSEing.

So I have written a new string constant pooling pass that does this. In order to make it lightweight I changed the syntax to allow multiple labels at definition site.

Such that I don't have to actually change the usage sites. I've changed all the back-ends but have a bug to fix and still need to figure out how to test the llvm, macos and unregister changes.

Last edited 9 months ago by Phyx- (previous) (diff)
Note: See TracTickets for help on using tickets.