Re: [PATCH] Basic asm blocks should always be volatile
My apologies, I sent this to the wrong list. I have already resent it to the correct one regards, Julian
Re: [PATCH] Basic asm blocks should always be volatile
On Tue, Jun 27, 2023 at 9:03 AM Julian Waters via Gcc 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
> 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
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 wrote:
> On Tue, Jun 27, 2023 at 9:03 AM Julian Waters via Gcc
> 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
> > 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
Perhaps this only affects compilation when GIMPLE isn't being used?
On Wed, Jun 28, 2023 at 12:15 AM Julian Waters
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 wrote:
>
>> On Tue, Jun 27, 2023 at 9:03 AM Julian Waters via Gcc
>> 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
>> > 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
On Tue, Jun 27, 2023 at 9:15 AM Julian Waters 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 wrote:
>>
>> On Tue, Jun 27, 2023 at 9:03 AM Julian Waters via Gcc
>> 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
>> > 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
On Tue, Jun 27, 2023 at 9:16 AM Julian Waters 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
> 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 wrote:
>>>
>>> On Tue, Jun 27, 2023 at 9:03 AM Julian Waters via Gcc
>>> 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
>>> > 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
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 wrote:
> On Tue, Jun 27, 2023 at 9:15 AM Julian Waters
> 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
> wrote:
> >>
> >> On Tue, Jun 27, 2023 at 9:03 AM Julian Waters via Gcc
> 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
> >> > 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
On Wed, Jun 28, 2023 at 12:32 AM Julian Waters 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 wrote:
>>
>> On Tue, Jun 27, 2023 at 9:15 AM Julian Waters
>> 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 wrote:
>> >>
>> >> On Tue, Jun 27, 2023 at 9:03 AM Julian Waters via Gcc
>> >> 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
>> >> > 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
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 wrote:
> On Wed, Jun 28, 2023 at 12:32 AM Julian Waters
> 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
> wrote:
> >>
> >> On Tue, Jun 27, 2023 at 9:15 AM Julian Waters
> 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
> 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
> >> >> > Date: Tue, 27 Jun 2023 23:56:38 +0800
> >> >> > Subject: [PATCH] asm not using extended syntax should always be
> volatile
> >> >> >
> >> >> > ---
> >> >> > gcc/cp/parser
Re: [PATCH] Basic asm blocks should always be volatile
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
On Wed, Jun 28, 2023 at 8:04 AM Michael Matz 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
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
0001400028c0 :
1400028c0: 48 83 ec 28 subrsp,0x28
1400028c4: e8 37 ec ff ffcall 140001500 <__main>
1400028c9: 90nop
1400028ca: 90nop
1400028cb: 90nop
1400028cc: 31 c0 xoreax,eax
1400028cd: 48 83 c4 28 addrsp,0x28
1400028ce: c3ret
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 wrote:
> On Wed, Jun 28, 2023 at 8:04 AM Michael Matz 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
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
>
> 0001400028c0 :
>1400028c0: 48 83 ec 28 subrsp,0x28
>1400028c4: e8 37 ec ff ffcall 140001500 <__main>
>1400028c9: 90nop
>1400028ca: 90nop
>1400028cb: 90nop
>1400028cc: 31 c0 xoreax,eax
>1400028cd: 48 83 c4 28 addrsp,0x28
>1400028ce: c3ret
>
> 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
...
00401020 :
401020: 90 nop
401021: 90 nop
401022: 90 nop
401023: 90 nop
401024: 90 nop
401025: 90 nop
401026: 31 c0 xoreax,eax
401028: c3 ret
401029: 0f 1f 80 00 00 00 00nopDWORD 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
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 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
> >
> > 0001400028c0 :
> >1400028c0: 48 83 ec 28 subrsp,0x28
> >1400028c4: e8 37 ec ff ffcall 140001500 <__main>
> >1400028c9: 90nop
> >1400028ca: 90nop
> >1400028cb: 90nop
> >1400028cc: 31 c0 xoreax,eax
> >1400028cd: 48 83 c4 28 addrsp,0x28
> >1400028ce: c3ret
> >
> > 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
> ...
> 00401020 :
> 401020: 90 nop
> 401021: 90 nop
> 401022: 90 nop
> 401023: 90 nop
> 401024: 90 nop
> 401025: 90 nop
> 401026: 31 c0 xoreax,eax
> 401028: c3 ret
> 401029: 0f 1f 80 00 00 00 00nopDWORD 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.
>
