[Bug middle-end/80053] Label with address taken should prevent duplication of containing basic block

[Bug middle-end/80053] Label with address taken should prevent duplication of containing basic block

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |INVALID

--- Comment #8 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
The manual does say:
You may not use this mechanism to jump to code in a different function. If you
do that, totally unpredictable things happen. The best way to avoid this is to
store the label address only in automatic variables and never pass it as an
argument.


And even says (which was added back in r0-94877 which was in 2009):
The &&foo expressions for the same label might have different values if the
containing function is inlined or cloned. If a program relies on them being
always the same, __attribute__((__noinline__,__noclone__)) should be used to
prevent inlining and cloning. If &&foo is used in a static variable
initializer, inlining and cloning is forbidden.

So I think we can say we document the label as values extension in a decent
way. Just you want to abuse it more.

[Bug middle-end/80053] Label with address taken should prevent duplication of containing basic block

[amonakov at gcc dot gnu.org]

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

Alexander Monakov <amonakov at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2021-07-24
         Resolution|INVALID                     |---
             Status|RESOLVED                    |NEW
     Ever confirmed|0                           |1

--- Comment #9 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
The documentation you're pointing to makes the testcase from comment #2
invalid, and I agree that's the right solution (address of a label remains
valid only as long as its containing function has not returned, similar to
automatic variables), but what is going on in comment #0 is still broken,
please don't close this bug.

[Bug middle-end/80053] Label with address taken should prevent duplication of containing basic block

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |INVALID

--- Comment #10 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
In comment #0 there is no computed goto and there for the label can go
anywhere.

See PR 28581.

There is a good solution on how to fix the code from breaking in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101559#c4 too.

[Bug middle-end/80053] Label with address taken should prevent duplication of containing basic block

[amonakov at gcc dot gnu.org]

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

Alexander Monakov <amonakov at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|INVALID                     |---
             Status|RESOLVED                    |NEW

--- Comment #11 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
In the comment you're pointing to Jakub admits that the compiler should account
for asm goto, which is exactly what the testcase is using.

More importantly, the crux of the issue is not whether I can produce a testcase
that satisfies you. The issue is that GIMPLE is (for no good reason as far as
I'm aware) less strict about this than RTL: RTL forbids duplication of such
blocks, GIMPLE does not. There's no explanation anywhere why the lax GIMPLE
behavior is not going to cause miscompilations. Please understand that and
don't rush to close.

[Bug middle-end/80053] Label with address taken should prevent duplication of containing basic block

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
That is not true, even RTL is moving such labels around, we have
NOTE_DELETED_LABEL for that after all.
The &&label extension has been added for computed goto and from the beginning
if the optimizers managed to optimize away all computed gotos that could jump
to a particular label it has been desirable that such left over labels aren't
optimization barriers.

[Bug middle-end/80053] Label with address taken should prevent duplication of containing basic block

[amonakov at gcc dot gnu.org]

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

--- Comment #13 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
Yes, I'm talking only about labels which are potential branch targets, of
course after the jumps have been DCE'd it is not really observable where the
label points to. Unfortunately after four years I do not remember which line in
the RTL machinery made me think RTL is careful about their containing blocks.

Okay, so the main motivation appears to be this: the address of a label may be
passed down to some callee, and that callee couldn't possibly use it in a goto.
So GCC doesn't want to pin down such labels and their containing blocks. This
makes sense.

Effectively GCC wants to distinguish two kinds of labels, ones that can
potentially be used as a goto target in the current function, and those that
can't.

I am not clear though if each pass is supposed to be careful about
computed-goto-like constructs (both plain and asm goto) independently, instead
of having can_duplicate_bb_p cfg hook return false for blocks with
jumpable+addressable labels (and then just using that hook in the pass)? My
testcase forced a miscompilation in the unswitching pass, but other passes
duplicate blocks too, and they may be similarly "vulnerable".

[Bug middle-end/80053] Label with address taken should prevent duplication of containing basic block

[rguenth at gcc dot gnu.org]

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

--- Comment #14 from Richard Biener <rguenth at gcc dot gnu.org> ---
I think the original asm goto case clearly remains and this is a difficult to
handle case since the label address only appears as regular input and the
goto target is statically represented in the CFG.  The testcase is miscompiled
at -O2 already.

I think asm goto is prone to such miscompilation in general if combined with
label addresses as inputs.  I don't think it was supposed to be used in this
way so we might want to simply amend documentation to make such uses undefined
...  in fact one might read

"The
@var{GotoLabels} section in an @code{asm goto} statement contains
a comma-separated
list of all C labels to which the assembler code may jump."

that jumps must jump to one of the labels literally (in the way documented
later).

[Bug middle-end/80053] Label with address taken should prevent duplication of containing basic block

[amonakov at gcc dot gnu.org]

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

--- Comment #15 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #14)
> I think the original asm goto case clearly remains and this is a difficult
> to handle case since the label address only appears as regular input and the
> goto target is statically represented in the CFG.  The testcase is
> miscompiled at -O2 already.
> 
> I think asm goto is prone to such miscompilation in general if combined with
> label addresses as inputs.  I don't think it was supposed to be used in this
> way so we might want to simply amend documentation to make such uses
> undefined ...  in fact one might read
> 
> "The
> @var{GotoLabels} section in an @code{asm goto} statement contains
> a comma-separated
> list of all C labels to which the assembler code may jump."
> 
> that jumps must jump to one of the labels literally (in the way documented
> later).

I don't see a contradiction? 'lp' holds the address of 'l'; label 'l' is listed
in the asm. It doesn't jump to anywhere but 'l'.

[Bug middle-end/80053] asm goto interacting with computed gotos and taking address of label extensions

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Label with address taken    |asm goto interacting with
                   |should prevent duplication  |computed gotos and taking
                   |of containing basic block   |address of label extensions
           Keywords|                            |documentation

--- Comment #16 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I think this interaction should be documented as being invalid and that is it.
As we already describe that requirement of a computed goto for address of
labels to be in the correct location. I think treating an inline-asm goto as a
computed goto is not very useful; maybe if there is an new feature to would be
have a way to treat some inline-asm goto's as a computed goto might be useful.