[Bug middle-end/80053] Label with address taken should prevent duplication of containing basic block
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80053
--- Comment #15 from Alexander Monakov ---
(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] Label with address taken should prevent duplication of containing basic block
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80053
--- Comment #14 from Richard Biener ---
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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80053 --- Comment #13 from Alexander Monakov --- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80053 Jakub Jelinek changed: What|Removed |Added CC||jakub at gcc dot gnu.org --- Comment #12 from Jakub Jelinek --- That is not true, even RTL is moving such labels around, we have NOTE_DELETED_LABEL for that after all. The & 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80053 Alexander Monakov changed: What|Removed |Added Resolution|INVALID |--- Status|RESOLVED|NEW --- Comment #11 from Alexander Monakov --- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80053 Andrew Pinski changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |INVALID --- Comment #10 from Andrew Pinski --- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80053 Alexander Monakov changed: What|Removed |Added Last reconfirmed||2021-07-24 Resolution|INVALID |--- Status|RESOLVED|NEW Ever confirmed|0 |1 --- Comment #9 from Alexander Monakov --- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80053 Andrew Pinski changed: What|Removed |Added Status|UNCONFIRMED |RESOLVED Resolution|--- |INVALID --- Comment #8 from Andrew Pinski --- 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 & 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 & 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80053 --- Comment #7 from Richard Biener --- I didnt' want to say you are wrong just had some thoughts that there may be cases where cloning/copying is ok.
[Bug middle-end/80053] Label with address taken should prevent duplication of containing basic block
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80053 --- Comment #6 from Alexander Monakov --- (In reply to rguent...@suse.de from comment #4) > To the value in the other BB/function. This works if the jump > targets are semantically compatible. For function cloning it's > probably hard to say as things like frame allocation may be > incompatible. For BB level it might be possible. ... but the whole point of the original testcase is to show that loop unswitching makes targets incompatible? Now I'm confused as to what you've meant, because afaics what you describe is what already happens in gcc: when the BB is cloned, exactly one of the copies retains the original label, which remains to be used for initialization of 'lp'.
[Bug middle-end/80053] Label with address taken should prevent duplication of containing basic block
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80053 --- Comment #5 from rguenther at suse dot de --- On Fri, 17 Mar 2017, amonakov at gcc dot gnu.org wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80053 > > --- Comment #3 from Alexander Monakov --- > ... unless labels are intended to act similar to non-static function-scope > variables, with computed address usable only until the containing function > returns? Except when used in static initializers, which makes them act as if > their addresses were constant expressions? Not sure - we do have nonlocal gotos (but those are introduced by means of nested functions and thus follow the static initialization rule). Certainly an interesting testcase ;) I suppose we simply forget to set DECL_NONLOCAL on L1/L2, doing so might pessimize the case where we have computed goto with local labels only though. > (still, if the this testcase is deemed invalid, the original issue remains)
[Bug middle-end/80053] Label with address taken should prevent duplication of containing basic block
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80053
--- Comment #4 from rguenther at suse dot de ---
On Thu, 16 Mar 2017, amonakov at gcc dot gnu.org wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80053
>
> --- Comment #2 from Alexander Monakov ---
> (In reply to Richard Biener from comment #1)
> > The question is whether the transform at hand is valid if the label is
> > duplicated
> > but all referers still refer to the original one (so if the label is dropped
> > at duplication time).
>
> Well, you still need to initialize 'lp' somehow, so I don't follow.
To the value in the other BB/function. This works if the jump
targets are semantically compatible. For function cloning it's
probably hard to say as things like frame allocation may be
incompatible. For BB level it might be possible.
> > The current handling for cloning is certainly too conservative.
>
> How so? I find that it's not conservative enough: it's possible to break it
> even without resorting to inline asm! The following testcase is miscompiled
> at
> -O3, because thanks to function cloning the .constprop clone will try to jump
> to the general implementation. Their epilogues are incompatible, so it
> manifests with a crash.
Interesting ;) And worth a separate bug.
> #include
> __attribute__((noinline, noclone))
> static int noop(char *c)
> {
> return 1;
> }
> __attribute__((noinline))
> static int cloneme(int p, void **target)
> {
> if (p == -1)
> *target = &
> if (p == -2)
> *target = &
> if (p < 0)
> return 0;
> char *tmp = 0;
> if (p) {
> puts("p != 0");
> tmp = __builtin_alloca(p);
> } else
> puts("p == 0");
> goto **target;
> L1:
> return 0;
> L2:
> return noop(tmp);
> }
>
> int main(int argc, char *argv[])
> {
> void *target1, *target2;
> cloneme(-1, );
> cloneme(-2, );
> for (int i=0; i<10; i++)
> cloneme(0, );
> cloneme(argc, );
> }
>
>
[Bug middle-end/80053] Label with address taken should prevent duplication of containing basic block
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80053 --- Comment #3 from Alexander Monakov --- ... unless labels are intended to act similar to non-static function-scope variables, with computed address usable only until the containing function returns? Except when used in static initializers, which makes them act as if their addresses were constant expressions? (still, if the this testcase is deemed invalid, the original issue remains)
[Bug middle-end/80053] Label with address taken should prevent duplication of containing basic block
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80053
--- Comment #2 from Alexander Monakov ---
(In reply to Richard Biener from comment #1)
> The question is whether the transform at hand is valid if the label is
> duplicated
> but all referers still refer to the original one (so if the label is dropped
> at duplication time).
Well, you still need to initialize 'lp' somehow, so I don't follow.
> The current handling for cloning is certainly too conservative.
How so? I find that it's not conservative enough: it's possible to break it
even without resorting to inline asm! The following testcase is miscompiled at
-O3, because thanks to function cloning the .constprop clone will try to jump
to the general implementation. Their epilogues are incompatible, so it
manifests with a crash.
#include
__attribute__((noinline, noclone))
static int noop(char *c)
{
return 1;
}
__attribute__((noinline))
static int cloneme(int p, void **target)
{
if (p == -1)
*target = &
if (p == -2)
*target = &
if (p < 0)
return 0;
char *tmp = 0;
if (p) {
puts("p != 0");
tmp = __builtin_alloca(p);
} else
puts("p == 0");
goto **target;
L1:
return 0;
L2:
return noop(tmp);
}
int main(int argc, char *argv[])
{
void *target1, *target2;
cloneme(-1, );
cloneme(-2, );
for (int i=0; i<10; i++)
cloneme(0, );
cloneme(argc, );
}
[Bug middle-end/80053] Label with address taken should prevent duplication of containing basic block
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80053 Richard Biener changed: What|Removed |Added CC||rguenth at gcc dot gnu.org --- Comment #1 from Richard Biener --- The question is whether the transform at hand is valid if the label is duplicated but all referers still refer to the original one (so if the label is dropped at duplication time). The current handling for cloning is certainly too conservative. Not sure if we want to extend this to can_copy_bb, it certainly would be consistent this way.
