This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Fix miscompilation in output_constant_def

<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.  */

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 of TREE_CONSTANT_POOL_ADDRESS.



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]