BUMP: AW: [PATCH] Microblaze: Type confusion in fprintf
Yes, it is a 32-bit host and 32-bit target.
HOST_WIDE_INT is long long (on ARM).
Also it says 4 lines above that the type of value_long is unsigned long, so
lets use the specifier for unsigned long.
Jonas
-Ursprüngliche Nachricht-
Von: Segher Boessenkool
Gesendet: Sonntag, 22. September 2019 02:52
An: Jeff Law
Cc: Jonas Pfeil ; gcc-patches@gcc.gnu.org
Betreff: Re: [PATCH] Microblaze: Type confusion in fprintf
On Fri, Sep 20, 2019 at 04:50:12PM -0600, Jeff Law wrote:
> On 9/12/19 7:33 AM, Jonas Pfeil wrote:
> > A type confusion leads to illegal (and nonsensical) assembly on
> > certain host architectures (e.g. ARM), where HOST_WIDE_INT (64 bit)
> > and unsigned long (32
> > bit) have different alignments. So this has printed an uninitialized
> > register instead of the intended value. This fixes float constant
> > initialization on an ARM->Microblaze cross compiler.
> >
> > --- a/gcc/config/microblaze/microblaze.c
> > +++ b/gcc/config/microblaze/microblaze.c
> > @@ -2468,7 +2468,7 @@ print_operand (FILE * file, rtx op, int letter)
> > unsigned long value_long;
> > REAL_VALUE_TO_TARGET_SINGLE (*CONST_DOUBLE_REAL_VALUE (op),
> >value_long);
> > - fprintf (file, HOST_WIDE_INT_PRINT_HEX, value_long);
> > + fprintf (file, "%#lx", value_long);
> > }
> >else
> > {
> >
> But shouldn't HOST_WIDE_INT_PRINT_HEX here result in the same thing?
>
> >From hwint.h:
>
> #ifndef HAVE_INTTYPES_H
> #if INT64_T_IS_LONG
> # define GCC_PRI64 HOST_LONG_FORMAT
> #else
> # define GCC_PRI64 HOST_LONG_LONG_FORMAT #endif
>
> [ ... ]
>
> #undef PRIx64
> #define PRIx64 GCC_PRI64 "x"
> [ ... ]
>
> #define HOST_WIDE_INT_PRINT_HEX "%#" PRIx64
>
>
> If that's not giving us back %#lx, then it seems to me like something
> is wrong elsewhere.
This is a 32-bit target though, so INT64_T_IS_LONG is false.
Segher
Fix for type confusion bug on microblaze
The Microblaze target confuses unsigned long with HOST_WIDE_INT.
This works fine for many architectures, but fails on ARM (HOST_WIDE_INT is 8
bytes, unsigned long is 4 bytes). Leading to print a uninitialized register
instead of the desired number, fix by using the correct printf-specifier.
Tested to fix the issue and work with an ARM->MB cross compiler.
--- a/gcc/config/microblaze/microblaze.h
+++ b/gcc/config/microblaze/microblaze.h
@@ -695,7 +695,7 @@ do {
\
fprintf (STREAM, "\t.align\t%d\n", (LOG))
#define ASM_OUTPUT_SKIP(STREAM,SIZE) \
- fprintf (STREAM, "\t.space\t%lu\n", (SIZE))
+ fprintf (STREAM, "\t.space\t" HOST_WIDE_INT_PRINT_DEC "\n", (SIZE))
#define ASCII_DATA_ASM_OP "\t.ascii\t"
#define STRING_ASM_OP "\t.asciz\t"
AW: Fix for type confusion bug on microblaze
Yes, you are correct. Tested it and it works as intended.
Thanks,
Jonas
--- a/gcc/config/microblaze/microblaze.h
+++ b/gcc/config/microblaze/microblaze.h
@@ -695,7 +695,7 @@ do {
\
fprintf (STREAM, "\t.align\t%d\n", (LOG))
#define ASM_OUTPUT_SKIP(STREAM,SIZE) \
- fprintf (STREAM, "\t.space\t%lu\n", (SIZE))
+ fprintf (STREAM, "\t.space\t" HOST_WIDE_INT_PRINT_UNSIGNED "\n", (SIZE))
#define ASCII_DATA_ASM_OP "\t.ascii\t"
#define STRING_ASM_OP "\t.asciz\t"
-Ursprüngliche Nachricht-
Von: Jeff Law
Gesendet: Mittwoch, 11. September 2019 19:33
An: Jonas Pfeil ; gcc-patches@gcc.gnu.org
Betreff: Re: Fix for type confusion bug on microblaze
On 9/11/19 5:28 AM, Jonas Pfeil wrote:
> The Microblaze target confuses unsigned long with HOST_WIDE_INT.
>
> This works fine for many architectures, but fails on ARM
> (HOST_WIDE_INT is 8 bytes, unsigned long is 4 bytes). Leading to print
> a uninitialized register instead of the desired number, fix by using the
> correct printf-specifier.
>
> Tested to fix the issue and work with an ARM->MB cross compiler.
>
> --- a/gcc/config/microblaze/microblaze.h
> +++ b/gcc/config/microblaze/microblaze.h
> @@ -695,7 +695,7 @@ do {
> \
>fprintf (STREAM, "\t.align\t%d\n", (LOG))
>
> #define ASM_OUTPUT_SKIP(STREAM,SIZE) \
> - fprintf (STREAM, "\t.space\t%lu\n", (SIZE))
> + fprintf (STREAM, "\t.space\t" HOST_WIDE_INT_PRINT_DEC "\n", (SIZE))
I believe that we should be using HOST_WIDE_INT_PRINT_UNSIGNED, not
HOST_WIDE_INT_PRINT_DEC. Can you please verify that works for your cross
builds?
Thanks,
jeff
[PATCH] Microblaze: Type confusion in fprintf
A type confusion leads to illegal (and nonsensical) assembly on certain host
architectures (e.g. ARM), where HOST_WIDE_INT (64 bit) and unsigned long (32
bit) have different alignments. So this has printed an uninitialized
register instead of the intended value. This fixes float constant
initialization on an ARM->Microblaze cross compiler.
--- a/gcc/config/microblaze/microblaze.c
+++ b/gcc/config/microblaze/microblaze.c
@@ -2468,7 +2468,7 @@ print_operand (FILE * file, rtx op, int letter)
unsigned long value_long;
REAL_VALUE_TO_TARGET_SINGLE (*CONST_DOUBLE_REAL_VALUE (op),
value_long);
- fprintf (file, HOST_WIDE_INT_PRINT_HEX, value_long);
+ fprintf (file, "%#lx", value_long);
}
else
{
