This is the mail archive of the
mailing list for the GCC project.
Re: [PATCH] Fix miscompilation in output_constant_def
- From: Maxim Kuvyrkov <maxim at codesourcery dot com>
- To: Jeff Law <law at redhat dot com>
- Cc: meloun at miracle dot cz, gcc-patches <gcc-patches at gcc dot gnu dot org>
- Date: Tue, 23 Jun 2009 20:41:42 +0400
- Subject: Re: [PATCH] Fix miscompilation in output_constant_def
- References: <491DB2C6.7658.C2CA2E3@meloun.miracle.cz> <email@example.com> <492C4A04.firstname.lastname@example.org>
<Sorry for the long delay>
Jeff Law wrote:
Maxim Kuvyrkov wrote:
--- varasm.c.orig 2008-11-14 18:04:27.693643900 +0100
+++ varasm.c 2008-11-14 17:58:06.522748300 +0100
@@ -3245,7 +3245,7 @@
maybe_output_constant_def_contents (desc, defer);
- return desc->rtl;
+ return copy_rtx (desc->rtl);
This patch seems correct to me. It makes output_constant_def() always
return a copy of the value rather than the value.
I will regtest this patch on ColdFire [where the miscompilation
occurs], wound anyone volunteer to test it on x86[_64]?
Unfortunately, there is no clear documentation on sharing of constant
pool entries; however, upon reviewing the code in GCC itself, it appears
to me the compiler expects entries in the constant pool to be shared.
That would make your patch incorrect.
Instead we need to focus on the code which reloads the symbol into a
register transforming (mem (symbol)) into (mem ((reg)). That code
should be copying the MEM before modifying it. If you search for
CONSTANT_POOL_ADDRESS in reload.c you'll see an example of what I mean.
After some more investigation I'm not sure that your conclusions are
correct. Indeed CONSTANT_POOL_ADDRESS entries are expected to be shared
and reload properly copies those before changing anything inside.
But here we have a TREE_CONSTANT_POOL_ADDRESS entry; the difference
between the two is that the former is used for function-scope constants
and the later is used for file-scope constants; the entries between the
pools do not mix, it is either the one or the other. The second, more
relevant to us right now difference, is that TREE_CONSTANT_POOL_ADDRESS
entries are handled exclusively in varasm.c:
/* 1 if RTX is a symbol_ref that addresses a value in the file's
tree constant pool. This information is private to varasm.c. */
#define TREE_CONSTANT_POOL_ADDRESS_P(RTX) ...
This can only be achieved by returning copies of memory references.
FWIW, I can patch up reload, but that doesn't look to be in the spirit