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

[Julian Waters <tanksherman27@gmail.com>]

[-- 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
>