[v1] configure: Improve alias attribute check

[PATCH] configure: Improve alias attribute check

Posted by Gavin Shan 3 years, 1 month ago

It's still possible that the wrong value is returned from the alias
of variable even if the program can be compiled without issue. This
improves the check by executing the binary to check the result.

If alias attribute can't be working properly, the @target_page in
exec-vary.c will always return zeroes when we have the following gcc
version.

  # gcc --version
  gcc (GCC) 11.0.0 20210210 (Red Hat 11.0.0-0)

This abstracts the code from exec-vary.c and use it as indicator to
enable gcc alias attribute or not.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 configure | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/configure b/configure
index f7d022a5db..8321f380d5 100755
--- a/configure
+++ b/configure
@@ -75,6 +75,7 @@ fi
 
 TMPB="qemu-conf"
 TMPC="${TMPDIR1}/${TMPB}.c"
+TMPC_B="${TMPDIR1}/${TMPB}_b.c"
 TMPO="${TMPDIR1}/${TMPB}.o"
 TMPCXX="${TMPDIR1}/${TMPB}.cxx"
 TMPE="${TMPDIR1}/${TMPB}.exe"
@@ -4878,13 +4879,38 @@ fi
 
 attralias=no
 cat > $TMPC << EOF
-int x = 1;
+static int x;
 extern const int y __attribute__((alias("x")));
-int main(void) { return 0; }
+extern int read_y(void);
+void write_x(int val);
+
+void write_x(int val)
+{
+    x = val;
+}
+
+int main(void)
+{
+    return read_y();
+}
 EOF
-if compile_prog "" "" ; then
-    attralias=yes
+cat > $TMPC_B << EOF
+extern const int y;
+extern void write_x(int val);
+int read_y(void);
+
+int read_y(void)
+{
+     write_x(1);
+     return y;
+}
+EOF
+
+TMPC+=" ${TMPC_B}"
+if compile_prog "" "" && ! $TMPE; then
+   attralias=yes
 fi
+TMPC="${TMPDIR1}/${TMPB}.c"
 
 ########################################
 # check if getauxval is available.
-- 
2.23.0

Re: [PATCH] configure: Improve alias attribute check

Posted by Paolo Bonzini 3 years, 1 month ago

On 20/03/21 05:27, Gavin Shan wrote:
> It's still possible that the wrong value is returned from the alias
> of variable even if the program can be compiled without issue. This
> improves the check by executing the binary to check the result.
> 
> If alias attribute can't be working properly, the @target_page in
> exec-vary.c will always return zeroes when we have the following gcc
> version.
> 
>   # gcc --version
>   gcc (GCC) 11.0.0 20210210 (Red Hat 11.0.0-0)
> 
> This abstracts the code from exec-vary.c and use it as indicator to
> enable gcc alias attribute or not.
> 
> +void write_x(int val);
> +
> +void write_x(int val)
> +{
> +    x = val;
> +}
> +
> +int main(void)
> +{
> +    return read_y();
> +}

I think this should be "read_y() == 1 ? 0 : 1".

I can reproduce it with -flto -O2 but not without -flto, do you agree?

Perhaps we can obtain the same optimization by wrapping reads of the 
page size in an inline __attribute__((const)) function.  Richard, what 
do you think?

Paolo

Re: [PATCH] configure: Improve alias attribute check

Posted by Richard Henderson 3 years, 1 month ago

On 3/20/21 11:52 AM, Paolo Bonzini wrote:
>> +int main(void)
>> +{
>> +    return read_y();
>> +}
> 
> I think this should be "read_y() == 1 ? 0 : 1".

As a testcase returning 0 on success, yes.

> I can reproduce it with -flto -O2 but not without -flto, do you agree?

Agreed.  Replicated with a random recent gcc 11 snapshot.
This is really annoying of lto.  It's clear something needs to change though.

> Perhaps we can obtain the same optimization by wrapping reads of the page size 
> in an inline __attribute__((const)) function.  Richard, what do you think?

I'll give it a shot and see what happens.

r~

Re: [PATCH] configure: Improve alias attribute check

Posted by Gavin Shan 3 years, 1 month ago

Hi Paolo and Richard,

On 3/21/21 9:33 AM, Richard Henderson wrote:
> On 3/20/21 11:52 AM, Paolo Bonzini wrote:
>>> +int main(void)
>>> +{
>>> +    return read_y();
>>> +}
>>
>> I think this should be "read_y() == 1 ? 0 : 1".
> 
> As a testcase returning 0 on success, yes.
> 

Ok. I will include the changes in v2. Also, I will
wrap the lines, for example:

int main(void) { return (read_y() == 1) ? 0 : 1; }

if compile_prog "" "" && $TMPE >/dev/null 2>/dev/null; then
    attralias=yes
fi

>> I can reproduce it with -flto -O2 but not without -flto, do you agree?
> 
> Agreed.  Replicated with a random recent gcc 11 snapshot.
> This is really annoying of lto.  It's clear something needs to change though.
> 

The command I used is:

gcc -O2 -flto=auto config-temp.c config-temp-b.c -o config-temp.exe.

Removing "-O2" or "-flto=auto" can make the gcc alias attribute workable again.

>> Perhaps we can obtain the same optimization by wrapping reads of the page size in an inline __attribute__((const)) function.  Richard, what do you think?
> 
> I'll give it a shot and see what happens.
> 

Thanks,
Gavin

Re: [PATCH] configure: Improve alias attribute check

Posted by Richard Henderson 3 years, 1 month ago

On 3/20/21 4:33 PM, Richard Henderson wrote:
> On 3/20/21 11:52 AM, Paolo Bonzini wrote:
>>> +int main(void)
>>> +{
>>> +    return read_y();
>>> +}
>>
>> I think this should be "read_y() == 1 ? 0 : 1".
> 
> As a testcase returning 0 on success, yes.
> 
>> I can reproduce it with -flto -O2 but not without -flto, do you agree?
> 
> Agreed.  Replicated with a random recent gcc 11 snapshot.
> This is really annoying of lto.  It's clear something needs to change though.
> 
>> Perhaps we can obtain the same optimization by wrapping reads of the page 
>> size in an inline __attribute__((const)) function.  Richard, what do you think?
> 
> I'll give it a shot and see what happens.

What exact version of gcc are you guys using?  Something from rawhide that I 
can just install?

So far I have failed to compile with gcc master with --enable-lto.  Lots of 
missing symbols reported at link time.  Therefore I've been unable to actually 
test what I intended to test.

That said, I'm not hopeful that __attribute__((const)) will work.  I have an 
idea that early inlining will inline tiny function calls too soon, before the 
attribute has a change to DTRT during CSE.  At which point we're left with bare 
variable references and we're exactly where we would be if we did nothing 
special at all.

Another workaround may be to avoid compiling exec-vary.c with -flto.  I'm not 
sure that my meson fu is up to that.  Paolo?

I have filed a gcc bug report:

   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99696

Hopefully someone can address that before gcc 11 gets released.  At which point 
we need do nothing in qemu.  Aldy?

r~

Re: [PATCH] configure: Improve alias attribute check

Posted by Paolo Bonzini 3 years, 1 month ago

Il dom 21 mar 2021, 16:49 Richard Henderson <richard.henderson@linaro.org>
ha scritto:

> What exact version of gcc are you guys using?  Something from rawhide that
> I can just install?
>

I am using Fedora 34. I upgraded just to test this bug and it seems stable
except that GNOME Shell extensions need an upgrade. However I haven't tried
building all of QEMU, only the test case.

So far I have failed to compile with gcc master with --enable-lto.  Lots of
> missing symbols reported at link time.  Therefore I've been unable to
> actually
> test what I intended to test.
>
> That said, I'm not hopeful that __attribute__((const)) will work.  I have
> an
> idea that early inlining will inline tiny function calls too soon, before
> the
> attribute has a change to DTRT during CSE.


Yeah that's at least plausible.

Another workaround may be to avoid compiling exec-vary.c with -flto.  I'm
> not
> sure that my meson fu is up to that.  Paolo?
>

You would have to define a static library.

I have filed a gcc bug report:
>
>    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99696
>
> Hopefully someone can address that before gcc 11 gets released.  At which
> point we need do nothing in qemu.  Aldy?


Good point, I can give it a shot too just to see how rusty I am... That
would be the best outcome, though we would have to check LLVM as well. If
const doesn't work it would indeed be prudent to include Gavin's configure
check.

Paolo


>
> r~
>
>

Re: [PATCH] configure: Improve alias attribute check

Posted by Richard Henderson 3 years, 1 month ago

On 3/21/21 10:50 AM, Paolo Bonzini wrote:
>     Another workaround may be to avoid compiling exec-vary.c with -flto.  I'm not
>     sure that my meson fu is up to that.  Paolo?
> 
> You would have to define a static library.

Ok.  With an extra -fno-lto flag, or can I somehow remove -flto from the 
library's cflags?  Or unset the meson b_lto variable?

>     I have filed a gcc bug report:
> 
>     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99696
>     <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99696>
> 
>     Hopefully someone can address that before gcc 11 gets released.  At which
>     point we need do nothing in qemu.  Aldy?
> 
> 
> Good point, I can give it a shot too just to see how rusty I am... That would 
> be the best outcome, though we would have to check LLVM as well. If const 
> doesn't work it would indeed be prudent to include Gavin's configure check.

So, I've reproduced the testcase failure with gcc 9.3 (ubuntu 20.04) as well. 
Which means that there are at least two releases for which this has not worked.

I think Gavin's runtime test is unnecessary.  We don't have to check the 
runtime results, we can just [ "$lto" = true ], and we fairly well know it'll fail.

r~

Re: [PATCH] configure: Improve alias attribute check

Posted by Paolo Bonzini 3 years, 1 month ago

Il dom 21 mar 2021, 18:34 Richard Henderson <richard.henderson@linaro.org>
ha scritto:

> On 3/21/21 10:50 AM, Paolo Bonzini wrote:
> >     Another workaround may be to avoid compiling exec-vary.c with
> -flto.  I'm not
> >     sure that my meson fu is up to that.  Paolo?
> >
> > You would have to define a static library.
>
> Ok.  With an extra -fno-lto flag, or can I somehow remove -flto from the
> library's cflags?  Or unset the meson b_lto variable?
>

-fno-lto should work, yes. b_lto tries to cater to other compilers, but we
don't support anything but gcc-like drivers.

>     I have filed a gcc bug report:
> >
> >     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99696
> >
> >     Hopefully someone can address that before gcc 11 gets released.  At
> which
> >     point we need do nothing in qemu.  Aldy?
>
> So, I've reproduced the testcase failure with gcc 9.3 (ubuntu 20.04) as
> well.
> Which means that there are at least two releases for which this has not
> worked.
>
> I think Gavin's runtime test is unnecessary.  We don't have to check the
> runtime results, we can just [ "$lto" = true ], and we fairly well know
> it'll fail.
>

Yeah, if anything the test can be used to re-enable attribute((alias)) once
we know there are some fixed compilers. (Though it's quite ugly to have
worse compilation when cross-compiling).

Paolo


>
> r~
>
>

Re: [PATCH] configure: Improve alias attribute check

Posted by Paolo Bonzini 3 years, 1 month ago

HRM, what about biting the bullet and making exec-vary.c a C++ source?...
Then instead of making it conditional an attribute((alias)), we make it
conditional on having a C++ compiler.

Making cpu-all.h compile as C++ would be complex, but we can isolate all
the required declarations in a separate header file that does not need
either osdep.h or glib-compat.h, and basically just have a global
constructor in exec-vary.cc that forwards to a function in exec.c.

Paolo

Il dom 21 mar 2021, 18:43 Paolo Bonzini <pbonzini@redhat.com> ha scritto:

>
>
> Il dom 21 mar 2021, 18:34 Richard Henderson <richard.henderson@linaro.org>
> ha scritto:
>
>> On 3/21/21 10:50 AM, Paolo Bonzini wrote:
>> >     Another workaround may be to avoid compiling exec-vary.c with
>> -flto.  I'm not
>> >     sure that my meson fu is up to that.  Paolo?
>> >
>> > You would have to define a static library.
>>
>> Ok.  With an extra -fno-lto flag, or can I somehow remove -flto from the
>> library's cflags?  Or unset the meson b_lto variable?
>>
>
> -fno-lto should work, yes. b_lto tries to cater to other compilers, but we
> don't support anything but gcc-like drivers.
>
> >     I have filed a gcc bug report:
>> >
>> >     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99696
>> >
>> >     Hopefully someone can address that before gcc 11 gets released.  At
>> which
>> >     point we need do nothing in qemu.  Aldy?
>>
>> So, I've reproduced the testcase failure with gcc 9.3 (ubuntu 20.04) as
>> well.
>> Which means that there are at least two releases for which this has not
>> worked.
>>
>> I think Gavin's runtime test is unnecessary.  We don't have to check the
>> runtime results, we can just [ "$lto" = true ], and we fairly well know
>> it'll fail.
>>
>
> Yeah, if anything the test can be used to re-enable attribute((alias))
> once we know there are some fixed compilers. (Though it's quite ugly to
> have worse compilation when cross-compiling).
>
> Paolo
>
>
>>
>> r~
>>
>>

Re: [PATCH] configure: Improve alias attribute check

Posted by Richard Henderson 3 years, 1 month ago

On 3/21/21 11:46 AM, Paolo Bonzini wrote:
> HRM, what about biting the bullet and making exec-vary.c a C++ source?... Then 
> instead of making it conditional an attribute((alias)), we make it conditional 
> on having a C++ compiler.

Doesn't help.  The gcc bug I filed talks about c++, because that's the closest 
analogy.

But set_preferred_target_page_bits is called *much* later than a constructor. 
Though still before any use of the variable in question, for which we have an 
--enable-debug-tcg assertion.


r~

Re: [PATCH] configure: Improve alias attribute check

Posted by Gavin Shan 3 years, 1 month ago

Hi Richard and Paolo,

On 3/22/21 5:23 AM, Richard Henderson wrote:
> On 3/21/21 11:46 AM, Paolo Bonzini wrote:
>> HRM, what about biting the bullet and making exec-vary.c a C++ source?... Then instead of making it conditional an attribute((alias)), we make it conditional on having a C++ compiler.
> 
> Doesn't help.  The gcc bug I filed talks about c++, because that's the closest analogy.
> 
> But set_preferred_target_page_bits is called *much* later than a constructor. Though still before any use of the variable in question, for which we have an --enable-debug-tcg assertion.
> 

It looks this issue can be avoided after "volatile" is applied to
@target_page. However, I'm not sure if it's the correct fix to have.
If it is, I can post a formal patch so that it can be included.

--- a/exec-vary.c
+++ b/exec-vary.c
-extern const TargetPageBits target_page
+extern const TargetPageBits volatile target_page
      __attribute__((alias("init_target_page")));
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
-extern const TargetPageBits target_page;
+extern const TargetPageBits volatile target_page;


According to the experiments I did, it would be function call
to set_preferred_target_page_bits() is dropped when the machine
is created. The following c files are used in the experiment:

--- a.c ---

static int x;
const extern int VOLATILE y __attribute__((alias("x")));
extern int read_y(void);

void write_x(int val) { x = 1; }
int main(void) { return read_y(); }

--- b.c---

extern const int VOLATILE y;
extern void write_x(int val);

int read_y(void) { write_x(1); return y; }

# gcc a.c b.c -O2 -flto=auto -DVOLATILE= -o a; ./a; echo $?
0
# gdb -nw ./a
(gdb) disassem main
Dump of assembler code for function main:
    0x0000000000400480 <+0>:	adrp	x1, 0x420000 <__libc_start_main@got.plt>
    0x0000000000400484 <+4>:	mov	w2, #0x1                   	// #1
    0x0000000000400488 <+8>:	mov	w0, #0x0                   	// #0
    0x000000000040048c <+12>:	str	w2, [x1, #32]
    0x0000000000400490 <+16>:	ret
End of assembler dump.

# gcc a.c b.c -O2 -flto=auto -DVOLATILE=volatile -o a; ./a; echo $?
1
# gdb -nw ./a
(gdb) disassem main
Dump of assembler code for function main:
    0x0000000000400480 <+0>:	adrp	x1, 0x420000 <__libc_start_main@got.plt>
    0x0000000000400484 <+4>:	mov	w2, #0x1                   	// #1
    0x0000000000400488 <+8>:	adrp	x0, 0x420000 <__libc_start_main@got.plt>
    0x000000000040048c <+12>:	str	w2, [x1, #32]
    0x0000000000400490 <+16>:	ldr	w0, [x0, #32]
    0x0000000000400494 <+20>:	ret


Thanks,
Gavin

Re: [PATCH] configure: Improve alias attribute check

Posted by Richard Henderson 3 years, 1 month ago

On 3/22/21 4:54 AM, Gavin Shan wrote:
> It looks this issue can be avoided after "volatile" is applied to
> @target_page. However, I'm not sure if it's the correct fix to have.

Certainly not.

That is the exact opposite of what we want.  We want to minimize the number of 
reads from the variable, not maximize them.

r~

Re: [PATCH] configure: Improve alias attribute check

Posted by Gavin Shan 3 years, 1 month ago

Hi Richard,

On 3/23/21 7:59 AM, Richard Henderson wrote:
> On 3/22/21 4:54 AM, Gavin Shan wrote:
>> It looks this issue can be avoided after "volatile" is applied to
>> @target_page. However, I'm not sure if it's the correct fix to have.
> 
> Certainly not.
> 
> That is the exact opposite of what we want.  We want to minimize the number of reads from the variable, not maximize them.
> 

Yes, It's something I was thinking of. "volatile" can make
@target_page visible to gcc, but maximizes the number of
reads. By the way, your patch to use "-fno-lto" worked for
me and it has been split into 3 patches by Phil. Richard,
thanks for the quick fixup :)

Thanks,
Gavin

Re: [PATCH] configure: Improve alias attribute check

Posted by Thomas Huth 3 years, 1 month ago

On 20/03/2021 05.27, Gavin Shan wrote:
> It's still possible that the wrong value is returned from the alias
> of variable even if the program can be compiled without issue. This
> improves the check by executing the binary to check the result.
> 
> If alias attribute can't be working properly, the @target_page in
> exec-vary.c will always return zeroes when we have the following gcc
> version.
> 
>    # gcc --version
>    gcc (GCC) 11.0.0 20210210 (Red Hat 11.0.0-0)
> 
> This abstracts the code from exec-vary.c and use it as indicator to
> enable gcc alias attribute or not.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>   configure | 34 ++++++++++++++++++++++++++++++----
>   1 file changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/configure b/configure
> index f7d022a5db..8321f380d5 100755
> --- a/configure
> +++ b/configure
> @@ -75,6 +75,7 @@ fi
>   
>   TMPB="qemu-conf"
>   TMPC="${TMPDIR1}/${TMPB}.c"
> +TMPC_B="${TMPDIR1}/${TMPB}_b.c"
>   TMPO="${TMPDIR1}/${TMPB}.o"
>   TMPCXX="${TMPDIR1}/${TMPB}.cxx"
>   TMPE="${TMPDIR1}/${TMPB}.exe"
> @@ -4878,13 +4879,38 @@ fi
>   
>   attralias=no
>   cat > $TMPC << EOF
> -int x = 1;
> +static int x;
>   extern const int y __attribute__((alias("x")));
> -int main(void) { return 0; }
> +extern int read_y(void);
> +void write_x(int val);
> +
> +void write_x(int val)
> +{
> +    x = val;
> +}
> +
> +int main(void)
> +{
> +    return read_y();
> +}
>   EOF
> -if compile_prog "" "" ; then
> -    attralias=yes
> +cat > $TMPC_B << EOF
> +extern const int y;
> +extern void write_x(int val);
> +int read_y(void);
> +
> +int read_y(void)
> +{
> +     write_x(1);
> +     return y;
> +}
> +EOF
> +
> +TMPC+=" ${TMPC_B}"
> +if compile_prog "" "" && ! $TMPE; then

What about cross-compiling? Running an executable won't work if QEMU gets 
cross-compiled...

  Thomas

Re: [PATCH] configure: Improve alias attribute check

Posted by Gavin Shan 3 years, 1 month ago

Hi Thomas,

On 3/20/21 3:48 PM, Thomas Huth wrote:
> On 20/03/2021 05.27, Gavin Shan wrote:
>> It's still possible that the wrong value is returned from the alias
>> of variable even if the program can be compiled without issue. This
>> improves the check by executing the binary to check the result.
>>
>> If alias attribute can't be working properly, the @target_page in
>> exec-vary.c will always return zeroes when we have the following gcc
>> version.
>>
>>    # gcc --version
>>    gcc (GCC) 11.0.0 20210210 (Red Hat 11.0.0-0)
>>
>> This abstracts the code from exec-vary.c and use it as indicator to
>> enable gcc alias attribute or not.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   configure | 34 ++++++++++++++++++++++++++++++----
>>   1 file changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/configure b/configure
>> index f7d022a5db..8321f380d5 100755
>> --- a/configure
>> +++ b/configure
>> @@ -75,6 +75,7 @@ fi
>>   TMPB="qemu-conf"
>>   TMPC="${TMPDIR1}/${TMPB}.c"
>> +TMPC_B="${TMPDIR1}/${TMPB}_b.c"
>>   TMPO="${TMPDIR1}/${TMPB}.o"
>>   TMPCXX="${TMPDIR1}/${TMPB}.cxx"
>>   TMPE="${TMPDIR1}/${TMPB}.exe"
>> @@ -4878,13 +4879,38 @@ fi
>>   attralias=no
>>   cat > $TMPC << EOF
>> -int x = 1;
>> +static int x;
>>   extern const int y __attribute__((alias("x")));
>> -int main(void) { return 0; }
>> +extern int read_y(void);
>> +void write_x(int val);
>> +
>> +void write_x(int val)
>> +{
>> +    x = val;
>> +}
>> +
>> +int main(void)
>> +{
>> +    return read_y();
>> +}
>>   EOF
>> -if compile_prog "" "" ; then
>> -    attralias=yes
>> +cat > $TMPC_B << EOF
>> +extern const int y;
>> +extern void write_x(int val);
>> +int read_y(void);
>> +
>> +int read_y(void)
>> +{
>> +     write_x(1);
>> +     return y;
>> +}
>> +EOF
>> +
>> +TMPC+=" ${TMPC_B}"
>> +if compile_prog "" "" && ! $TMPE; then
> 
> What about cross-compiling? Running an executable won't work if QEMU gets cross-compiled...
> 

Executing the cross-compiled binary returns 126, which means we will
disable gcc alias attribute for cross-compiling case with the following
changes included into v2:

int main(void) { return (read_y() == 1) ? 0 : 1; }

if compile_prog "" "" && $TMPE >/dev/null 2>/dev/null; then
    attralias=yes
fi

Thanks,
Gavin