[m-rev.] trivial diff: workaround cygwin C compiler bug

[Fergus Henderson]

I've discussed this problem with the GCC developers, and the concensus
seems to that although the GCC documentation is not entirely clear,
GCC is behaving as intended here.

So the best solution that I can think of is to change the way that the
Mercury compiler generates code so that it generates something which
GCC will think is distinct at each label -- for example, a different
assembler comment.

Details and patch follow...

On 29-Jan-2003, Fergus Henderson <fjh at cs.mu.OZ.AU> wrote:
> On 29-Jan-2003, Mark Brown <dougl at cs.mu.OZ.AU> wrote:
> > Wait, I think I see the problem.  We use the code addresses of those labels
> > to look up the label layout info in a hash table, with the assumption that
> > different labels will have different code addresses.  In this case,
> > though, the code pointed to by the two labels happens to be identical,
> > and the C compiler appears to recognise this and (quite rightly) merge the
> > two pieces of code into one.
> 
> Hmm, interesting.
> 
> The C standard guarantees that "Two pointers compare equal if and only if
> both are null pointers, both are pointers to the same object (...)
> or function, [... or some cases involving arrays ...]".
> This means that pointers to the same function don't compare equal,
> even if the functions happen to do the same thing.
> 
> However, the GCC documentation of its address-of-labels extension
> is silent on the issue of equality of label addresses.

I have discussed this one with the GCC developers, and the concensus is
that GCC should leave this explicitly unspecified.

> Here's a small C program that demonstrates the issue.
> Compiled with `-O0', it works fine, but when compiled with `-O2',
> it gets an assertion failure.  (This is with gcc 3.1.)
> 
> 	#include <assert.h>
> 	int i;
> 	void foo() {}
> 	int main() {
> 		void * labels[] = { &&label0, &&label1, &&label2, &&label3 };
> 		goto *labels[i];
> 		label0:
> 			assert(labels[2] != labels[3]);
> 		label1:
> 			return 0;
> 		label2:
> 			foo();
> 			goto label0;
> 		label3:
> 			foo();
> 			goto label0;
> 
> 	}
> 
> Note that the assertion fails even if there is an `asm volatile("...");'
> before each call to foo(), as is the case with the code that the Mercury
> compiler generates in grade asm_fast.gc.  This optimization seems to
> contradict the GCC documentation about "asm volatile" statements not
> being "deleted, moved significantly, or combined".

I have also discussed this with the GCC developers.
Apparently the documentation about "asm volatile" statements not
being combined is intended to relate only to the dynamic execution trace,
not to the static occurrences in the generated code.  So this
optimization is permitted.

So the best solution that I can think of is to change the way that the
Mercury compiler generates code so that it generates something which
GCC will think is distinct at each label -- for example, a different
assembler comment.

Now, the difficulty with that is that assembler comment syntax might vary
between different assemblers.  However, I have verified that C-style
"/* ... */" comments inside GCC "asm" statements are supported on the
following systems:

		Linux (i386, alpha, sparc)
		MacOS X (powerpc)
		Solaris (i386, sparc)
		Windows (i386)

So that looks portable enough.  I've modified the configure script
so that the asm_* grades will only be supported if C-style comments
are supported in GCC "asm" statements.

Hence the following patch.  This patch puts an assembler comment after
every label, regardless of whether we actually need identity of labels
or not.  I don't know how much of a performance impact this will have.
It might be nicer to only add the assembler comment if tracing or
accurate GC or anything else that depends on labels having unique
addresses is enabled.  But that would complicate things, and make
them more difficult to maintain.  The LLDS back-end is such a pain to
maintain already that I'm not too fussed about the performance of asm_*
grades -- I'd almost rather they got slower, so that we ditch them sooner ;-)

----------

Estimated hours taken: 5
Branches: main

Fix a bug that broke tests/debugger/declarative/output_term_dep
with `--opt-space' when using gcc 3.1.

runtime/mercury_goto.h:
	Change MR_ASM_LOCAL_ENTRY() so that we insert the label name as
	an assembler comment after the label.  This is needed to prevent
	GCC from making optimizations which merge labels that point to
	the same code, but for which we have different stack layouts, etc.

configure.in:
	Add a "/* ... */" comment to the inline assembler in the code
	fragment that we use for checking whether asm labels are supported.

Workspace: /home/ceres/fjh/mercury
Index: configure.in
===================================================================
RCS file: /home/mercury1/repository/mercury/configure.in,v
retrieving revision 1.349
diff -u -d -r1.349 configure.in
--- configure.in	28 Jan 2003 01:53:56 -0000	1.349
+++ configure.in	3 Feb 2003 07:11:47 -0000
@@ -1806,7 +1806,8 @@
 
 	mercury__label1:
 		__asm__(".globl entry_" "mercury__label1" "\n" "entry_"
-			"mercury__label1" ":");
+			"mercury__label1" ":\n"
+			"/* this is a comment */");
 		exit(0);
 	}], [mercury_cv_asm_labels=yes], [mercury_cv_asm_labels=no],
 		[mercury_cv_asm_labels=no])
Index: runtime/mercury_goto.h
===================================================================
RCS file: /home/mercury1/repository/mercury/runtime/mercury_goto.h,v
retrieving revision 1.33
diff -u -d -r1.33 mercury_goto.h
--- runtime/mercury_goto.h	18 Feb 2002 07:01:15 -0000	1.33
+++ runtime/mercury_goto.h	3 Feb 2003 07:12:56 -0000
@@ -396,6 +396,15 @@
 #endif
 
 /*
+** MR_INLINE_ASM_COMMENT is an assembler string to
+** define an assembler comment.
+*/
+#ifndef MR_INLINE_ASM_COMMENT
+#define MR_INLINE_ASM_COMMENT(label)	\
+	"/* " MR_STRINGIFY(label) "*/\n"
+#endif
+
+/*
 ** MR_ASM_JUMP is used to jump to an entry point defined with
 ** MR_ASM_ENTRY, MR_ASM_STATIC_ENTRY, or MR_ASM_LOCAL_ENTRY.
 */
@@ -450,11 +459,20 @@
 ** although there won't be any direct calls to them from another
 ** C file, their address may be taken and so there may be indirect
 ** calls.
+**
+** We need to use MR_ASM_COMMENT here to ensure that
+** the code following each label is distinct; this is needed
+** to ensure GCC doesn't try to merge two labels which point
+** to identical code, but for which we have different information
+** stored in the stack layout structs, etc.
 */
 #define MR_ASM_LOCAL_ENTRY(label) 			\
 	MR_ASM_FALLTHROUGH(label)			\
   	MR_entry(label):				\
-  	MR_ASM_FIXUP_REGS				\
+	__asm__ __volatile__(				\
+		MR_INLINE_ASM_COMMENT(label)		\
+		MR_INLINE_ASM_FIXUP_REGS		\
+	);						\
 	MR_skip(label): ;
 
 /*---------------------------------------------------------------------------*/

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------