Re: [PATCH] Basic asm blocks should always be volatile

Re: [PATCH] Basic asm blocks should always be volatile

[Julian Waters]

[-- Attachment #1: Type: text/plain, Size: 106 bytes --]

My apologies, I sent this to the wrong list. I have already resent
it to the correct one

regards,
Julian

[PATCH] Basic asm blocks should always be volatile

[Julian Waters]

[-- Attachment #1: Type: text/plain, Size: 1146 bytes --]

gcc's documentatation mentions that all basic asm blocks are always volatile,
yet the parser fails to account for this by only ever setting
volatile_p to true
if the volatile qualifier is found. This patch fixes this by adding a
special case check for extended_p before finish_asm_statement is called

From 3094be39e3e65a6a638f05fafd858b89fefde6b5 Mon Sep 17 00:00:00 2001
From: TheShermanTanker <tanksherman27@gmail.com>
Date: Tue, 27 Jun 2023 23:56:38 +0800
Subject: [PATCH] asm not using extended syntax should always be volatile

---
 gcc/cp/parser.cc | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
index a6341b9..ef3d06a 100644
--- a/gcc/cp/parser.cc
+++ b/gcc/cp/parser.cc
@@ -22355,6 +22355,9 @@ cp_parser_asm_definition (cp_parser* parser)
       /* Create the ASM_EXPR.  */
       if (parser->in_function_body)
  {
+          if (!extended_p) {
+            volatile_p = true;
+          }
    asm_stmt = finish_asm_stmt (asm_loc, volatile_p, string, outputs,
        inputs, clobbers, labels, inline_p);
    /* If the extended syntax was not used, mark the ASM_EXPR.  */
-- 
2.35.1.windows.2

Re: [PATCH] Basic asm blocks should always be volatile

[Andrew Pinski]

On Tue, Jun 27, 2023 at 9:03 AM Julian Waters via Gcc <gcc@gcc.gnu.org> wrote:
>
> gcc's documentatation mentions that all basic asm blocks are always volatile,
> yet the parser fails to account for this by only ever setting
> volatile_p to true
> if the volatile qualifier is found. This patch fixes this by adding a
> special case check for extended_p before finish_asm_statement is called

The patch which are you doing will not change the behavior of GCC as
GCC already treats them as volatile later on.
non-extended inline-asm has no outputs so the following code in the
gimplifier will kick in and turn the gimple statement into volatile:
      gimple_asm_set_volatile (stmt, ASM_VOLATILE_P (expr) || noutputs == 0);

(note I am about to push a patch which changes the condition slightly
to have `asm goto` as volatile).

Thanks,
Andrew

>
> From 3094be39e3e65a6a638f05fafd858b89fefde6b5 Mon Sep 17 00:00:00 2001
> From: TheShermanTanker <tanksherman27@gmail.com>
> Date: Tue, 27 Jun 2023 23:56:38 +0800
> Subject: [PATCH] asm not using extended syntax should always be volatile
>
> ---
>  gcc/cp/parser.cc | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
> index a6341b9..ef3d06a 100644
> --- a/gcc/cp/parser.cc
> +++ b/gcc/cp/parser.cc
> @@ -22355,6 +22355,9 @@ cp_parser_asm_definition (cp_parser* parser)
>        /* Create the ASM_EXPR.  */
>        if (parser->in_function_body)
>   {
> +          if (!extended_p) {
> +            volatile_p = true;
> +          }
>     asm_stmt = finish_asm_stmt (asm_loc, volatile_p, string, outputs,
>         inputs, clobbers, labels, inline_p);
>     /* If the extended syntax was not used, mark the ASM_EXPR.  */
> --
> 2.35.1.windows.2

Re: [PATCH] Basic asm blocks should always be volatile

[Julian Waters]

[-- Attachment #1: Type: text/plain, Size: 2222 bytes --]

Hi Andrew,

That can't be right, on my system a test of asm vs asm volatile with -O3
and -flto=auto yields very different results, with only the latter being
correct. The patch fixed it and caused gcc to emit correct assembly

best regards,
Julian

On Wed, Jun 28, 2023 at 12:08 AM Andrew Pinski <pinskia@gmail.com> wrote:

> On Tue, Jun 27, 2023 at 9:03 AM Julian Waters via Gcc <gcc@gcc.gnu.org>
> wrote:
> >
> > gcc's documentatation mentions that all basic asm blocks are always
> volatile,
> > yet the parser fails to account for this by only ever setting
> > volatile_p to true
> > if the volatile qualifier is found. This patch fixes this by adding a
> > special case check for extended_p before finish_asm_statement is called
>
> The patch which are you doing will not change the behavior of GCC as
> GCC already treats them as volatile later on.
> non-extended inline-asm has no outputs so the following code in the
> gimplifier will kick in and turn the gimple statement into volatile:
>       gimple_asm_set_volatile (stmt, ASM_VOLATILE_P (expr) || noutputs ==
> 0);
>
> (note I am about to push a patch which changes the condition slightly
> to have `asm goto` as volatile).
>
> Thanks,
> Andrew
>
> >
> > From 3094be39e3e65a6a638f05fafd858b89fefde6b5 Mon Sep 17 00:00:00 2001
> > From: TheShermanTanker <tanksherman27@gmail.com>
> > Date: Tue, 27 Jun 2023 23:56:38 +0800
> > Subject: [PATCH] asm not using extended syntax should always be volatile
> >
> > ---
> >  gcc/cp/parser.cc | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
> > index a6341b9..ef3d06a 100644
> > --- a/gcc/cp/parser.cc
> > +++ b/gcc/cp/parser.cc
> > @@ -22355,6 +22355,9 @@ cp_parser_asm_definition (cp_parser* parser)
> >        /* Create the ASM_EXPR.  */
> >        if (parser->in_function_body)
> >   {
> > +          if (!extended_p) {
> > +            volatile_p = true;
> > +          }
> >     asm_stmt = finish_asm_stmt (asm_loc, volatile_p, string, outputs,
> >         inputs, clobbers, labels, inline_p);
> >     /* If the extended syntax was not used, mark the ASM_EXPR.  */
> > --
> > 2.35.1.windows.2
>

Re: [PATCH] Basic asm blocks should always be volatile

[Julian Waters]

[-- Attachment #1: Type: text/plain, Size: 2450 bytes --]

Perhaps this only affects compilation when GIMPLE isn't being used?

On Wed, Jun 28, 2023 at 12:15 AM Julian Waters <tanksherman27@gmail.com>
wrote:

> Hi Andrew,
>
> That can't be right, on my system a test of asm vs asm volatile with -O3
> and -flto=auto yields very different results, with only the latter being
> correct. The patch fixed it and caused gcc to emit correct assembly
>
> best regards,
> Julian
>
> On Wed, Jun 28, 2023 at 12:08 AM Andrew Pinski <pinskia@gmail.com> wrote:
>
>> On Tue, Jun 27, 2023 at 9:03 AM Julian Waters via Gcc <gcc@gcc.gnu.org>
>> wrote:
>> >
>> > gcc's documentatation mentions that all basic asm blocks are always
>> volatile,
>> > yet the parser fails to account for this by only ever setting
>> > volatile_p to true
>> > if the volatile qualifier is found. This patch fixes this by adding a
>> > special case check for extended_p before finish_asm_statement is called
>>
>> The patch which are you doing will not change the behavior of GCC as
>> GCC already treats them as volatile later on.
>> non-extended inline-asm has no outputs so the following code in the
>> gimplifier will kick in and turn the gimple statement into volatile:
>>       gimple_asm_set_volatile (stmt, ASM_VOLATILE_P (expr) || noutputs ==
>> 0);
>>
>> (note I am about to push a patch which changes the condition slightly
>> to have `asm goto` as volatile).
>>
>> Thanks,
>> Andrew
>>
>> >
>> > From 3094be39e3e65a6a638f05fafd858b89fefde6b5 Mon Sep 17 00:00:00 2001
>> > From: TheShermanTanker <tanksherman27@gmail.com>
>> > Date: Tue, 27 Jun 2023 23:56:38 +0800
>> > Subject: [PATCH] asm not using extended syntax should always be volatile
>> >
>> > ---
>> >  gcc/cp/parser.cc | 3 +++
>> >  1 file changed, 3 insertions(+)
>> >
>> > diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
>> > index a6341b9..ef3d06a 100644
>> > --- a/gcc/cp/parser.cc
>> > +++ b/gcc/cp/parser.cc
>> > @@ -22355,6 +22355,9 @@ cp_parser_asm_definition (cp_parser* parser)
>> >        /* Create the ASM_EXPR.  */
>> >        if (parser->in_function_body)
>> >   {
>> > +          if (!extended_p) {
>> > +            volatile_p = true;
>> > +          }
>> >     asm_stmt = finish_asm_stmt (asm_loc, volatile_p, string, outputs,
>> >         inputs, clobbers, labels, inline_p);
>> >     /* If the extended syntax was not used, mark the ASM_EXPR.  */
>> > --
>> > 2.35.1.windows.2
>>
>

Re: [PATCH] Basic asm blocks should always be volatile

[Andrew Pinski]

On Tue, Jun 27, 2023 at 9:16 AM Julian Waters <tanksherman27@gmail.com> wrote:
>
> Perhaps this only affects compilation when GIMPLE isn't being used?

The only time GIMPLE is not used is if you supply -fsyntax-only so ...

Thanks,
Andrew

>
> On Wed, Jun 28, 2023 at 12:15 AM Julian Waters <tanksherman27@gmail.com> wrote:
>>
>> Hi Andrew,
>>
>> That can't be right, on my system a test of asm vs asm volatile with -O3 and -flto=auto yields very different results, with only the latter being correct. The patch fixed it and caused gcc to emit correct assembly
>>
>> best regards,
>> Julian
>>
>> On Wed, Jun 28, 2023 at 12:08 AM Andrew Pinski <pinskia@gmail.com> wrote:
>>>
>>> On Tue, Jun 27, 2023 at 9:03 AM Julian Waters via Gcc <gcc@gcc.gnu.org> wrote:
>>> >
>>> > gcc's documentatation mentions that all basic asm blocks are always volatile,
>>> > yet the parser fails to account for this by only ever setting
>>> > volatile_p to true
>>> > if the volatile qualifier is found. This patch fixes this by adding a
>>> > special case check for extended_p before finish_asm_statement is called
>>>
>>> The patch which are you doing will not change the behavior of GCC as
>>> GCC already treats them as volatile later on.
>>> non-extended inline-asm has no outputs so the following code in the
>>> gimplifier will kick in and turn the gimple statement into volatile:
>>>       gimple_asm_set_volatile (stmt, ASM_VOLATILE_P (expr) || noutputs == 0);
>>>
>>> (note I am about to push a patch which changes the condition slightly
>>> to have `asm goto` as volatile).
>>>
>>> Thanks,
>>> Andrew
>>>
>>> >
>>> > From 3094be39e3e65a6a638f05fafd858b89fefde6b5 Mon Sep 17 00:00:00 2001
>>> > From: TheShermanTanker <tanksherman27@gmail.com>
>>> > Date: Tue, 27 Jun 2023 23:56:38 +0800
>>> > Subject: [PATCH] asm not using extended syntax should always be volatile
>>> >
>>> > ---
>>> >  gcc/cp/parser.cc | 3 +++
>>> >  1 file changed, 3 insertions(+)
>>> >
>>> > diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
>>> > index a6341b9..ef3d06a 100644
>>> > --- a/gcc/cp/parser.cc
>>> > +++ b/gcc/cp/parser.cc
>>> > @@ -22355,6 +22355,9 @@ cp_parser_asm_definition (cp_parser* parser)
>>> >        /* Create the ASM_EXPR.  */
>>> >        if (parser->in_function_body)
>>> >   {
>>> > +          if (!extended_p) {
>>> > +            volatile_p = true;
>>> > +          }
>>> >     asm_stmt = finish_asm_stmt (asm_loc, volatile_p, string, outputs,
>>> >         inputs, clobbers, labels, inline_p);
>>> >     /* If the extended syntax was not used, mark the ASM_EXPR.  */
>>> > --
>>> > 2.35.1.windows.2

Re: [PATCH] Basic asm blocks should always be volatile

[Andrew Pinski]

On Tue, Jun 27, 2023 at 9:15 AM Julian Waters <tanksherman27@gmail.com> wrote:
>
> Hi Andrew,
>
> That can't be right, on my system a test of asm vs asm volatile with -O3 and -flto=auto yields very different results, with only the latter being correct. The patch fixed it and caused gcc to emit correct assembly

Can you provide a few testcases? Because the gimplifier should always happen.

Thanks,
Andrew Pinski

>
> best regards,
> Julian
>
> On Wed, Jun 28, 2023 at 12:08 AM Andrew Pinski <pinskia@gmail.com> wrote:
>>
>> On Tue, Jun 27, 2023 at 9:03 AM Julian Waters via Gcc <gcc@gcc.gnu.org> wrote:
>> >
>> > gcc's documentatation mentions that all basic asm blocks are always volatile,
>> > yet the parser fails to account for this by only ever setting
>> > volatile_p to true
>> > if the volatile qualifier is found. This patch fixes this by adding a
>> > special case check for extended_p before finish_asm_statement is called
>>
>> The patch which are you doing will not change the behavior of GCC as
>> GCC already treats them as volatile later on.
>> non-extended inline-asm has no outputs so the following code in the
>> gimplifier will kick in and turn the gimple statement into volatile:
>>       gimple_asm_set_volatile (stmt, ASM_VOLATILE_P (expr) || noutputs == 0);
>>
>> (note I am about to push a patch which changes the condition slightly
>> to have `asm goto` as volatile).
>>
>> Thanks,
>> Andrew
>>
>> >
>> > From 3094be39e3e65a6a638f05fafd858b89fefde6b5 Mon Sep 17 00:00:00 2001
>> > From: TheShermanTanker <tanksherman27@gmail.com>
>> > Date: Tue, 27 Jun 2023 23:56:38 +0800
>> > Subject: [PATCH] asm not using extended syntax should always be volatile
>> >
>> > ---
>> >  gcc/cp/parser.cc | 3 +++
>> >  1 file changed, 3 insertions(+)
>> >
>> > diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
>> > index a6341b9..ef3d06a 100644
>> > --- a/gcc/cp/parser.cc
>> > +++ b/gcc/cp/parser.cc
>> > @@ -22355,6 +22355,9 @@ cp_parser_asm_definition (cp_parser* parser)
>> >        /* Create the ASM_EXPR.  */
>> >        if (parser->in_function_body)
>> >   {
>> > +          if (!extended_p) {
>> > +            volatile_p = true;
>> > +          }
>> >     asm_stmt = finish_asm_stmt (asm_loc, volatile_p, string, outputs,
>> >         inputs, clobbers, labels, inline_p);
>> >     /* If the extended syntax was not used, mark the ASM_EXPR.  */
>> > --
>> > 2.35.1.windows.2

Re: [PATCH] Basic asm blocks should always be volatile

[Julian Waters]

[-- Attachment #1: Type: text/plain, Size: 3949 bytes --]

Hi Andrew,

On a Microsoft Windows target the following (placed inside a function of
course) will only work correctly if volatile is specified in the basic asm
block (or if the attached patch was applied to gcc):

    asm ("1:" "\n"
         "\t" ".seh_handler __C_specific_handler, @except" "\n"
         "\t" ".seh_handlerdata" "\n"
         "\t" ".long 1" "\n"
         "\t" ".rva 1b, 2f, 3f, 4f" "\n"
         "\t" ".seh_code");

    {
        // std::printf("Guarded\n");
        RaiseException(EXCEPTION_BREAKPOINT, 0, 0, nullptr);
    }

    asm ("nop" "\n"
         "\t" "2: nop" "\n"
         "\t" "jmp 5f" "\n"
         "\t" "3:" "\n"
         "\t" "push rbp" "\n"
         "\t" "mov rbp, rsp"
         "\t" "push r15" "\n"
         "\t" "mov r15, rcx" "\n");

    [] [[gnu::noinline, gnu::used]] () -> long {
        return EXCEPTION_EXECUTE_HANDLER;
    }();

    asm ("pop r15" "\n"
         "\t" "pop rbp" "\n"
         "\t" "ret" "\n"
         "\t" "nop" "\n"
         "4:");

    {
        std::printf("Exception\n");
    }

    asm ("5:");

In any case I doubt marking it as volatile in the parser hurts either,
since this is the behaviour it's supposed to have

best regards,
Julian

On Wed, Jun 28, 2023 at 12:24 AM Andrew Pinski <pinskia@gmail.com> wrote:

> On Tue, Jun 27, 2023 at 9:15 AM Julian Waters <tanksherman27@gmail.com>
> wrote:
> >
> > Hi Andrew,
> >
> > That can't be right, on my system a test of asm vs asm volatile with -O3
> and -flto=auto yields very different results, with only the latter being
> correct. The patch fixed it and caused gcc to emit correct assembly
>
> Can you provide a few testcases? Because the gimplifier should always
> happen.
>
> Thanks,
> Andrew Pinski
>
> >
> > best regards,
> > Julian
> >
> > On Wed, Jun 28, 2023 at 12:08 AM Andrew Pinski <pinskia@gmail.com>
> wrote:
> >>
> >> On Tue, Jun 27, 2023 at 9:03 AM Julian Waters via Gcc <gcc@gcc.gnu.org>
> wrote:
> >> >
> >> > gcc's documentatation mentions that all basic asm blocks are always
> volatile,
> >> > yet the parser fails to account for this by only ever setting
> >> > volatile_p to true
> >> > if the volatile qualifier is found. This patch fixes this by adding a
> >> > special case check for extended_p before finish_asm_statement is
> called
> >>
> >> The patch which are you doing will not change the behavior of GCC as
> >> GCC already treats them as volatile later on.
> >> non-extended inline-asm has no outputs so the following code in the
> >> gimplifier will kick in and turn the gimple statement into volatile:
> >>       gimple_asm_set_volatile (stmt, ASM_VOLATILE_P (expr) || noutputs
> == 0);
> >>
> >> (note I am about to push a patch which changes the condition slightly
> >> to have `asm goto` as volatile).
> >>
> >> Thanks,
> >> Andrew
> >>
> >> >
> >> > From 3094be39e3e65a6a638f05fafd858b89fefde6b5 Mon Sep 17 00:00:00 2001
> >> > From: TheShermanTanker <tanksherman27@gmail.com>
> >> > Date: Tue, 27 Jun 2023 23:56:38 +0800
> >> > Subject: [PATCH] asm not using extended syntax should always be
> volatile
> >> >
> >> > ---
> >> >  gcc/cp/parser.cc | 3 +++
> >> >  1 file changed, 3 insertions(+)
> >> >
> >> > diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
> >> > index a6341b9..ef3d06a 100644
> >> > --- a/gcc/cp/parser.cc
> >> > +++ b/gcc/cp/parser.cc
> >> > @@ -22355,6 +22355,9 @@ cp_parser_asm_definition (cp_parser* parser)
> >> >        /* Create the ASM_EXPR.  */
> >> >        if (parser->in_function_body)
> >> >   {
> >> > +          if (!extended_p) {
> >> > +            volatile_p = true;
> >> > +          }
> >> >     asm_stmt = finish_asm_stmt (asm_loc, volatile_p, string, outputs,
> >> >         inputs, clobbers, labels, inline_p);
> >> >     /* If the extended syntax was not used, mark the ASM_EXPR.  */
> >> > --
> >> > 2.35.1.windows.2
>

Re: [PATCH] Basic asm blocks should always be volatile

[Andrew Pinski]

On Wed, Jun 28, 2023 at 12:32 AM Julian Waters <tanksherman27@gmail.com> wrote:
>
> Hi Andrew,
>
> On a Microsoft Windows target the following (placed inside a function of course) will only work correctly if volatile is specified in the basic asm block (or if the attached patch was applied to gcc):

These inline-asm will never work correctly .... even with volatile
because you change the sp behind the back of GCC and the control flow
too.
Also I suspect you want gnu::noipa rather than gnu::noinline for the
lambda there as I suspect the IPA passes are getting rid of the
function call thinking it is just pure/const.
Rather than related to the inline-asm being volatile or not.

Thanks,
Andrew

>
>     asm ("1:" "\n"
>          "\t" ".seh_handler __C_specific_handler, @except" "\n"
>          "\t" ".seh_handlerdata" "\n"
>          "\t" ".long 1" "\n"
>          "\t" ".rva 1b, 2f, 3f, 4f" "\n"
>          "\t" ".seh_code");
>
>     {
>         // std::printf("Guarded\n");
>         RaiseException(EXCEPTION_BREAKPOINT, 0, 0, nullptr);
>     }
>
>     asm ("nop" "\n"
>          "\t" "2: nop" "\n"
>          "\t" "jmp 5f" "\n"
>          "\t" "3:" "\n"
>          "\t" "push rbp" "\n"
>          "\t" "mov rbp, rsp"
>          "\t" "push r15" "\n"
>          "\t" "mov r15, rcx" "\n");
>
>     [] [[gnu::noinline, gnu::used]] () -> long {
>         return EXCEPTION_EXECUTE_HANDLER;
>     }();
>
>     asm ("pop r15" "\n"
>          "\t" "pop rbp" "\n"
>          "\t" "ret" "\n"
>          "\t" "nop" "\n"
>          "4:");
>
>     {
>         std::printf("Exception\n");
>     }
>
>     asm ("5:");
>
> In any case I doubt marking it as volatile in the parser hurts either, since this is the behaviour it's supposed to have
>
> best regards,
> Julian
>
> On Wed, Jun 28, 2023 at 12:24 AM Andrew Pinski <pinskia@gmail.com> wrote:
>>
>> On Tue, Jun 27, 2023 at 9:15 AM Julian Waters <tanksherman27@gmail.com> wrote:
>> >
>> > Hi Andrew,
>> >
>> > That can't be right, on my system a test of asm vs asm volatile with -O3 and -flto=auto yields very different results, with only the latter being correct. The patch fixed it and caused gcc to emit correct assembly
>>
>> Can you provide a few testcases? Because the gimplifier should always happen.
>>
>> Thanks,
>> Andrew Pinski
>>
>> >
>> > best regards,
>> > Julian
>> >
>> > On Wed, Jun 28, 2023 at 12:08 AM Andrew Pinski <pinskia@gmail.com> wrote:
>> >>
>> >> On Tue, Jun 27, 2023 at 9:03 AM Julian Waters via Gcc <gcc@gcc.gnu.org> wrote:
>> >> >
>> >> > gcc's documentatation mentions that all basic asm blocks are always volatile,
>> >> > yet the parser fails to account for this by only ever setting
>> >> > volatile_p to true
>> >> > if the volatile qualifier is found. This patch fixes this by adding a
>> >> > special case check for extended_p before finish_asm_statement is called
>> >>
>> >> The patch which are you doing will not change the behavior of GCC as
>> >> GCC already treats them as volatile later on.
>> >> non-extended inline-asm has no outputs so the following code in the
>> >> gimplifier will kick in and turn the gimple statement into volatile:
>> >>       gimple_asm_set_volatile (stmt, ASM_VOLATILE_P (expr) || noutputs == 0);
>> >>
>> >> (note I am about to push a patch which changes the condition slightly
>> >> to have `asm goto` as volatile).
>> >>
>> >> Thanks,
>> >> Andrew
>> >>
>> >> >
>> >> > From 3094be39e3e65a6a638f05fafd858b89fefde6b5 Mon Sep 17 00:00:00 2001
>> >> > From: TheShermanTanker <tanksherman27@gmail.com>
>> >> > Date: Tue, 27 Jun 2023 23:56:38 +0800
>> >> > Subject: [PATCH] asm not using extended syntax should always be volatile
>> >> >
>> >> > ---
>> >> >  gcc/cp/parser.cc | 3 +++
>> >> >  1 file changed, 3 insertions(+)
>> >> >
>> >> > diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
>> >> > index a6341b9..ef3d06a 100644
>> >> > --- a/gcc/cp/parser.cc
>> >> > +++ b/gcc/cp/parser.cc
>> >> > @@ -22355,6 +22355,9 @@ cp_parser_asm_definition (cp_parser* parser)
>> >> >        /* Create the ASM_EXPR.  */
>> >> >        if (parser->in_function_body)
>> >> >   {
>> >> > +          if (!extended_p) {
>> >> > +            volatile_p = true;
>> >> > +          }
>> >> >     asm_stmt = finish_asm_stmt (asm_loc, volatile_p, string, outputs,
>> >> >         inputs, clobbers, labels, inline_p);
>> >> >     /* If the extended syntax was not used, mark the ASM_EXPR.  */
>> >> > --
>> >> > 2.35.1.windows.2

Re: [PATCH] Basic asm blocks should always be volatile

[Julian Waters]

[-- Attachment #1: Type: text/plain, Size: 6091 bytes --]

Hi Andrew,

On the contrary, code compiled with gcc with or without the applied patch
operates very differently, with only gcc with the applied patch producing a
fully correctly operating program as expected. Even if the above inline
assembly blocks never worked due to other optimizations however, the
failure mode of the program would be very different from how it fails now:
It should fail noisily with an access violation exception due to
the malformed assembly, but instead all the assembly which installs an
exception handler never makes it into the final program because with
anything higher than -O1 gcc deletes all of it (I have verified this with
objdump too), so the original point still stands: That gcc is not
correctly treating ISO C++ basic asm blocks as volatile. I'm sure a
different example than the current one I have now could be brought up, but
the fact that the volatile specifier is enough to change the behaviour of
the program when it should always be volatile should be enough evidence of
something being awry.

Thanks for the tip with the lambdas though, I'll do just that

best regards,
Julian

On Wed, Jun 28, 2023 at 3:39 PM Andrew Pinski <pinskia@gmail.com> wrote:

> On Wed, Jun 28, 2023 at 12:32 AM Julian Waters <tanksherman27@gmail.com>
> wrote:
> >
> > Hi Andrew,
> >
> > On a Microsoft Windows target the following (placed inside a function of
> course) will only work correctly if volatile is specified in the basic asm
> block (or if the attached patch was applied to gcc):
>
> These inline-asm will never work correctly .... even with volatile
> because you change the sp behind the back of GCC and the control flow
> too.
> Also I suspect you want gnu::noipa rather than gnu::noinline for the
> lambda there as I suspect the IPA passes are getting rid of the
> function call thinking it is just pure/const.
> Rather than related to the inline-asm being volatile or not.
>
> Thanks,
> Andrew
>
> >
> >     asm ("1:" "\n"
> >          "\t" ".seh_handler __C_specific_handler, @except" "\n"
> >          "\t" ".seh_handlerdata" "\n"
> >          "\t" ".long 1" "\n"
> >          "\t" ".rva 1b, 2f, 3f, 4f" "\n"
> >          "\t" ".seh_code");
> >
> >     {
> >         // std::printf("Guarded\n");
> >         RaiseException(EXCEPTION_BREAKPOINT, 0, 0, nullptr);
> >     }
> >
> >     asm ("nop" "\n"
> >          "\t" "2: nop" "\n"
> >          "\t" "jmp 5f" "\n"
> >          "\t" "3:" "\n"
> >          "\t" "push rbp" "\n"
> >          "\t" "mov rbp, rsp"
> >          "\t" "push r15" "\n"
> >          "\t" "mov r15, rcx" "\n");
> >
> >     [] [[gnu::noinline, gnu::used]] () -> long {
> >         return EXCEPTION_EXECUTE_HANDLER;
> >     }();
> >
> >     asm ("pop r15" "\n"
> >          "\t" "pop rbp" "\n"
> >          "\t" "ret" "\n"
> >          "\t" "nop" "\n"
> >          "4:");
> >
> >     {
> >         std::printf("Exception\n");
> >     }
> >
> >     asm ("5:");
> >
> > In any case I doubt marking it as volatile in the parser hurts either,
> since this is the behaviour it's supposed to have
> >
> > best regards,
> > Julian
> >
> > On Wed, Jun 28, 2023 at 12:24 AM Andrew Pinski <pinskia@gmail.com>
> wrote:
> >>
> >> On Tue, Jun 27, 2023 at 9:15 AM Julian Waters <tanksherman27@gmail.com>
> wrote:
> >> >
> >> > Hi Andrew,
> >> >
> >> > That can't be right, on my system a test of asm vs asm volatile with
> -O3 and -flto=auto yields very different results, with only the latter
> being correct. The patch fixed it and caused gcc to emit correct assembly
> >>
> >> Can you provide a few testcases? Because the gimplifier should always
> happen.
> >>
> >> Thanks,
> >> Andrew Pinski
> >>
> >> >
> >> > best regards,
> >> > Julian
> >> >
> >> > On Wed, Jun 28, 2023 at 12:08 AM Andrew Pinski <pinskia@gmail.com>
> wrote:
> >> >>
> >> >> On Tue, Jun 27, 2023 at 9:03 AM Julian Waters via Gcc <
> gcc@gcc.gnu.org> wrote:
> >> >> >
> >> >> > gcc's documentatation mentions that all basic asm blocks are
> always volatile,
> >> >> > yet the parser fails to account for this by only ever setting
> >> >> > volatile_p to true
> >> >> > if the volatile qualifier is found. This patch fixes this by
> adding a
> >> >> > special case check for extended_p before finish_asm_statement is
> called
> >> >>
> >> >> The patch which are you doing will not change the behavior of GCC as
> >> >> GCC already treats them as volatile later on.
> >> >> non-extended inline-asm has no outputs so the following code in the
> >> >> gimplifier will kick in and turn the gimple statement into volatile:
> >> >>       gimple_asm_set_volatile (stmt, ASM_VOLATILE_P (expr) ||
> noutputs == 0);
> >> >>
> >> >> (note I am about to push a patch which changes the condition slightly
> >> >> to have `asm goto` as volatile).
> >> >>
> >> >> Thanks,
> >> >> Andrew
> >> >>
> >> >> >
> >> >> > From 3094be39e3e65a6a638f05fafd858b89fefde6b5 Mon Sep 17 00:00:00
> 2001
> >> >> > From: TheShermanTanker <tanksherman27@gmail.com>
> >> >> > Date: Tue, 27 Jun 2023 23:56:38 +0800
> >> >> > Subject: [PATCH] asm not using extended syntax should always be
> volatile
> >> >> >
> >> >> > ---
> >> >> >  gcc/cp/parser.cc | 3 +++
> >> >> >  1 file changed, 3 insertions(+)
> >> >> >
> >> >> > diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
> >> >> > index a6341b9..ef3d06a 100644
> >> >> > --- a/gcc/cp/parser.cc
> >> >> > +++ b/gcc/cp/parser.cc
> >> >> > @@ -22355,6 +22355,9 @@ cp_parser_asm_definition (cp_parser*
> parser)
> >> >> >        /* Create the ASM_EXPR.  */
> >> >> >        if (parser->in_function_body)
> >> >> >   {
> >> >> > +          if (!extended_p) {
> >> >> > +            volatile_p = true;
> >> >> > +          }
> >> >> >     asm_stmt = finish_asm_stmt (asm_loc, volatile_p, string,
> outputs,
> >> >> >         inputs, clobbers, labels, inline_p);
> >> >> >     /* If the extended syntax was not used, mark the ASM_EXPR.  */
> >> >> > --
> >> >> > 2.35.1.windows.2
>

Re: [PATCH] Basic asm blocks should always be volatile

[Michael Matz]

Hello,

On Wed, 28 Jun 2023, Julian Waters via Gcc wrote:

> On the contrary, code compiled with gcc with or without the applied patch
> operates very differently, with only gcc with the applied patch producing a
> fully correctly operating program as expected. Even if the above inline
> assembly blocks never worked due to other optimizations however, the
> failure mode of the program would be very different from how it fails now:
> It should fail noisily with an access violation exception due to
> the malformed assembly, but instead all the assembly which installs an
> exception handler never makes it into the final program because with
> anything higher than -O1 gcc deletes all of it (I have verified this with
> objdump too),

Can you please provide a _full_ compilable testcase (preprocessed).  What 
Andrew says is (supposed to be) correct: ignoring the other 
problems you're going to see with your asms (even if you make them 
volatile) GCC should not remove any of the asm statements of them.

If something changes when you add 'volatile' by hand then we have another 
problem lurking somewhere, and adding the parser patch might not fully 
solve it (even if it changes behaviour for you).


Ciao,
Michael.

Re: [PATCH] Basic asm blocks should always be volatile

[Andrew Pinski]

On Wed, Jun 28, 2023 at 8:04 AM Michael Matz <matz@suse.de> wrote:
>
> Hello,
>
> On Wed, 28 Jun 2023, Julian Waters via Gcc wrote:
>
> > On the contrary, code compiled with gcc with or without the applied patch
> > operates very differently, with only gcc with the applied patch producing a
> > fully correctly operating program as expected. Even if the above inline
> > assembly blocks never worked due to other optimizations however, the
> > failure mode of the program would be very different from how it fails now:
> > It should fail noisily with an access violation exception due to
> > the malformed assembly, but instead all the assembly which installs an
> > exception handler never makes it into the final program because with
> > anything higher than -O1 gcc deletes all of it (I have verified this with
> > objdump too),
>
> Can you please provide a _full_ compilable testcase (preprocessed).  What
> Andrew says is (supposed to be) correct: ignoring the other
> problems you're going to see with your asms (even if you make them
> volatile) GCC should not remove any of the asm statements of them.
>
> If something changes when you add 'volatile' by hand then we have another
> problem lurking somewhere, and adding the parser patch might not fully
> solve it (even if it changes behaviour for you).

By the way I just testcase:
```
void f(void)
{
  asm("#should be volatile");
}
```

The produced gimple (via -fdump-tree-gimple=/dev/stdout) is:
```
void f ()
{
  __asm__ __volatile__("#should be volatile");
}
```

Which is 100% volatile. and I tested all the way back to GCC 4.8.0 and
it was volatile back then too.
So as both Michael and myself have mentioned, we need a full
(compilable) testcase, even if it is for mingw or cygwin, we can
handle those just fine too.

Thanks,
Andrew

>
>
> Ciao,
> Michael.

Re: [PATCH] Basic asm blocks should always be volatile

[Julian Waters]

[-- Attachment #1: Type: text/plain, Size: 3352 bytes --]

Hi Andrew,

Here is a simpler testcase, with the resulting assembly attached below

int main() {
    asm ("nop" "\n"
         "\t" "nop" "\n"
         "\t" "nop");

    asm volatile ("nop" "\n"
                  "\t" "nop" "\n"
                  "\t" "nop");
}

objdump --disassemble-all -M intel -M intel-mnemonic a.exe > disassembly.txt

00000001400028c0 <main>:
   1400028c0: 48 83 ec 28             sub    rsp,0x28
   1400028c4: e8 37 ec ff ff            call   140001500 <__main>
   1400028c9: 90                            nop
   1400028ca: 90                            nop
   1400028cb: 90                            nop
   1400028cc: 31 c0                       xor    eax,eax
   1400028cd: 48 83 c4 28             add    rsp,0x28
   1400028ce: c3                            ret

Note how there are only 3 nops above when there should be 6, as the first 3
have been deleted by the compiler. With the patch, the correct 6 nops
instead of 3 are compiled into the final code.

Of course, the above was compiled with the most extreme optimizations
available to stress test the possible bug, -O3, -ffunction-sections
-fdata-sections -Wl,--gc-sections -flto=auto. Compiled as C++17 and intel
assembly syntax

best regards,
Julian

On Thu, Jun 29, 2023 at 2:46 AM Andrew Pinski <pinskia@gmail.com> wrote:

> On Wed, Jun 28, 2023 at 8:04 AM Michael Matz <matz@suse.de> wrote:
> >
> > Hello,
> >
> > On Wed, 28 Jun 2023, Julian Waters via Gcc wrote:
> >
> > > On the contrary, code compiled with gcc with or without the applied
> patch
> > > operates very differently, with only gcc with the applied patch
> producing a
> > > fully correctly operating program as expected. Even if the above inline
> > > assembly blocks never worked due to other optimizations however, the
> > > failure mode of the program would be very different from how it fails
> now:
> > > It should fail noisily with an access violation exception due to
> > > the malformed assembly, but instead all the assembly which installs an
> > > exception handler never makes it into the final program because with
> > > anything higher than -O1 gcc deletes all of it (I have verified this
> with
> > > objdump too),
> >
> > Can you please provide a _full_ compilable testcase (preprocessed).  What
> > Andrew says is (supposed to be) correct: ignoring the other
> > problems you're going to see with your asms (even if you make them
> > volatile) GCC should not remove any of the asm statements of them.
> >
> > If something changes when you add 'volatile' by hand then we have another
> > problem lurking somewhere, and adding the parser patch might not fully
> > solve it (even if it changes behaviour for you).
>
> By the way I just testcase:
> ```
> void f(void)
> {
>   asm("#should be volatile");
> }
> ```
>
> The produced gimple (via -fdump-tree-gimple=/dev/stdout) is:
> ```
> void f ()
> {
>   __asm__ __volatile__("#should be volatile");
> }
> ```
>
> Which is 100% volatile. and I tested all the way back to GCC 4.8.0 and
> it was volatile back then too.
> So as both Michael and myself have mentioned, we need a full
> (compilable) testcase, even if it is for mingw or cygwin, we can
> handle those just fine too.
>
> Thanks,
> Andrew
>
> >
> >
> > Ciao,
> > Michael.
>

Re: [PATCH] Basic asm blocks should always be volatile

[Michael Matz]

Hello,

On Thu, 29 Jun 2023, Julian Waters via Gcc wrote:

> int main() {
>     asm ("nop" "\n"
>          "\t" "nop" "\n"
>          "\t" "nop");
> 
>     asm volatile ("nop" "\n"
>                   "\t" "nop" "\n"
>                   "\t" "nop");
> }
> 
> objdump --disassemble-all -M intel -M intel-mnemonic a.exe > disassembly.txt
> 
> 00000001400028c0 <main>:
>    1400028c0: 48 83 ec 28             sub    rsp,0x28
>    1400028c4: e8 37 ec ff ff            call   140001500 <__main>
>    1400028c9: 90                            nop
>    1400028ca: 90                            nop
>    1400028cb: 90                            nop
>    1400028cc: 31 c0                       xor    eax,eax
>    1400028cd: 48 83 c4 28             add    rsp,0x28
>    1400028ce: c3                            ret
> 
> Note how there are only 3 nops above when there should be 6, as the first 3
> have been deleted by the compiler. With the patch, the correct 6 nops
> instead of 3 are compiled into the final code.
> 
> Of course, the above was compiled with the most extreme optimizations
> available to stress test the possible bug, -O3, -ffunction-sections
> -fdata-sections -Wl,--gc-sections -flto=auto. Compiled as C++17 and intel
> assembly syntax

Works just fine here:

% cat xx.c
int main() {
    asm ("nop" "\n"
         "\t" "nop" "\n"
         "\t" "nop");

    asm volatile ("nop" "\n"
                  "\t" "nop" "\n"
                  "\t" "nop");
}

% g++ -v
...
gcc version 12.2.1 20230124 [revision 
193f7e62815b4089dfaed4c2bd34fd4f10209e27] (SUSE Linux)

% g++ -std=c++17 -flto=auto -O3 -ffunction-sections -fdata-sections xx.c
% objdump --disassemble-all -M intel -M intel-mnemonic a.out
...
0000000000401020 <main>:
  401020:       90                      nop
  401021:       90                      nop
  401022:       90                      nop
  401023:       90                      nop
  401024:       90                      nop
  401025:       90                      nop
  401026:       31 c0                   xor    eax,eax
  401028:       c3                      ret
  401029:       0f 1f 80 00 00 00 00    nop    DWORD PTR [rax+0x0]
...

Testing with recent trunk works as well with no differences in output.
This is for x86_64-linux.

So, as suspected, something else is broken for you.  Which compiler 
version are you using?  (And we need to check if it's something in the 
mingw target)


Ciao,
Michael.

Re: [PATCH] Basic asm blocks should always be volatile

[Julian Waters]

[-- Attachment #1: Type: text/plain, Size: 2856 bytes --]

Hi Michael,

I'm on gcc 13.1 compiling for Windows x64, with the MinGW UCRT64 runtime
library

best regards,
Julian

On Thu, Jun 29, 2023 at 9:27 PM Michael Matz <matz@suse.de> wrote:

> Hello,
>
> On Thu, 29 Jun 2023, Julian Waters via Gcc wrote:
>
> > int main() {
> >     asm ("nop" "\n"
> >          "\t" "nop" "\n"
> >          "\t" "nop");
> >
> >     asm volatile ("nop" "\n"
> >                   "\t" "nop" "\n"
> >                   "\t" "nop");
> > }
> >
> > objdump --disassemble-all -M intel -M intel-mnemonic a.exe >
> disassembly.txt
> >
> > 00000001400028c0 <main>:
> >    1400028c0: 48 83 ec 28             sub    rsp,0x28
> >    1400028c4: e8 37 ec ff ff            call   140001500 <__main>
> >    1400028c9: 90                            nop
> >    1400028ca: 90                            nop
> >    1400028cb: 90                            nop
> >    1400028cc: 31 c0                       xor    eax,eax
> >    1400028cd: 48 83 c4 28             add    rsp,0x28
> >    1400028ce: c3                            ret
> >
> > Note how there are only 3 nops above when there should be 6, as the
> first 3
> > have been deleted by the compiler. With the patch, the correct 6 nops
> > instead of 3 are compiled into the final code.
> >
> > Of course, the above was compiled with the most extreme optimizations
> > available to stress test the possible bug, -O3, -ffunction-sections
> > -fdata-sections -Wl,--gc-sections -flto=auto. Compiled as C++17 and intel
> > assembly syntax
>
> Works just fine here:
>
> % cat xx.c
> int main() {
>     asm ("nop" "\n"
>          "\t" "nop" "\n"
>          "\t" "nop");
>
>     asm volatile ("nop" "\n"
>                   "\t" "nop" "\n"
>                   "\t" "nop");
> }
>
> % g++ -v
> ...
> gcc version 12.2.1 20230124 [revision
> 193f7e62815b4089dfaed4c2bd34fd4f10209e27] (SUSE Linux)
>
> % g++ -std=c++17 -flto=auto -O3 -ffunction-sections -fdata-sections xx.c
> % objdump --disassemble-all -M intel -M intel-mnemonic a.out
> ...
> 0000000000401020 <main>:
>   401020:       90                      nop
>   401021:       90                      nop
>   401022:       90                      nop
>   401023:       90                      nop
>   401024:       90                      nop
>   401025:       90                      nop
>   401026:       31 c0                   xor    eax,eax
>   401028:       c3                      ret
>   401029:       0f 1f 80 00 00 00 00    nop    DWORD PTR [rax+0x0]
> ...
>
> Testing with recent trunk works as well with no differences in output.
> This is for x86_64-linux.
>
> So, as suspected, something else is broken for you.  Which compiler
> version are you using?  (And we need to check if it's something in the
> mingw target)
>
>
> Ciao,
> Michael.
>