[Bug c/110728] New: should __attribute__((cleanup())) callback get invoked for indirect edges of asm goto

[Bug c/110728] New: should __attribute__((cleanup())) callback get invoked for indirect edges of asm goto

[ndesaulniers at google dot com]

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

            Bug ID: 110728
           Summary: should __attribute__((cleanup())) callback get invoked
                    for indirect edges of asm goto
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: ndesaulniers at google dot com
  Target Milestone: ---

Consider the following test case:

```c
void test4cleanup(int*);
// No errors expected.
void test4(void) {
l0:;
    int x __attribute__((cleanup(test4cleanup)));
    asm goto("# %l0"::::l0);
}

```

gcc trunk today generates effectively the following control flow:
```
test4:
.LFB0:
        subq    $24, %rsp
.L2:
#APP
        # .L2
#NO_APP
        leaq    12(%rsp), %rdi
        call    test4cleanup
        addq    $24, %rsp
        ret
```
so if the inline asm blob jumps to `l0`, then the cleanup function is not run.

That seemed surprising, at least when we discussed it on this thread.
https://reviews.llvm.org/D155342#4511887

A fellow Linux kernel dev (who introduced the usage of
__attribute__((cleanup())) (and asm goto, coincidentally) to the kernel) agreed
(on IRC).

For now in clang, we produce a diagnostic since the behavior seems surprising. 
If this gets changed in GCC, I'd be happy to modify clang to match that updated
behavior.

[Bug c/110728] should __attribute__((cleanup())) callback get invoked for indirect edges of asm goto

[pinskia at gcc dot gnu.org]

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

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I suspect PR 91951 is the same really.

[Bug c/110728] should __attribute__((cleanup())) callback get invoked for indirect edges of asm goto

[pinskia at gcc dot gnu.org]

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

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Techincally x does not go out of scope until the } so I don't see how this
would act any other way.

[Bug c/110728] should __attribute__((cleanup())) callback get invoked for indirect edges of asm goto

[pinskia at gcc dot gnu.org]

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

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
That is even doing:
```
int g();
int h()
{
l0:;
    int x __attribute__((cleanup(test4cleanup)));
    if (g()) goto l0;
}
```
Produces the same result which is why I said this is the same as PR 91951
really.

[Bug c/110728] should __attribute__((cleanup())) callback get invoked for indirect edges of asm goto

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=37722

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Oh and computed goto has a similar issue too (PR 37722 which is for C++
deconstructors but cleanup is the same idea there).

[Bug c/110728] should __attribute__((cleanup())) callback get invoked for indirect edges of asm goto

[rjmccall at gmail dot com]

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

--- Comment #5 from John McCall <rjmccall at gmail dot com> ---
> If this gets changed in GCC, I'd be happy to modify clang to match that updated behavior.

Policy-wise, I don't think clang would accept a patch making this UB
(effectively what not calling the destructor/cleanup means) instead of
ill-formed unless a standards body forced us to.  Not calling the
destructor/cleanup seems like clearly undesirable behavior, and if we can
define that away in the compiler with relative ease, we should.

[Bug c/110728] should __attribute__((cleanup())) callback get invoked for indirect edges of asm goto

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2023-07-19
     Ever confirmed|0                           |1

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #3)
> That is even doing:
> ```
> int g();
> int h()
> {
> l0:;
>     int x __attribute__((cleanup(test4cleanup)));
>     if (g()) goto l0;
> }
> ```
> Produces the same result which is why I said this is the same as PR 91951
> really.

But the above works fine?

  <bb 2> :
l0:
  _1 = g ();
  if (_1 != 0)
    goto <bb 3>; [INV]
  else
    goto <bb 4>; [INV]

  <bb 3> :
  // predicted unlikely by goto predictor.
  finally_tmp.0 = 0;
  goto <bb 5>; [INV]

  <bb 4> :
  finally_tmp.0 = 1;

  <bb 5> :
  test4cleanup (&x);
  if (finally_tmp.0 == 1)
    goto <bb 6>; [INV]
  else
    goto <bb 2>; [INV]

  <bb 6> :
<L4>:

  <bb 7> :
  x = {CLOBBER(eol)};
  return;

so it goes wrong somewhere during EH lowering, possibly because asm goto
isn't handled there.  Confirmed.

[Bug middle-end/110728] should __attribute__((cleanup())) callback get invoked for indirect edges of asm goto

[ndesaulniers at google dot com]

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

--- Comment #7 from Nick Desaulniers <ndesaulniers at google dot com> ---
(In reply to Andrew Pinski from comment #1)
> I suspect PR 91951 is the same really.

PR 91951 seems to be about a missing diagnostic dependent on optimization
level.  This bug report is more so a question about codegen.

(In reply to Andrew Pinski from comment #2)
> Techincally x does not go out of scope until the } so I don't see how this
> would act any other way.

Compare the codegen with my example vs replacing the `asm goto (..., l0);` with
`goto l0;`.

With `goto`, the destructor is invoked before taking the backward edge.  With
`asm goto` it is not.  That inconsistency leads me to think there might be a
bug (with `asm goto`).

(In reply to Andrew Pinski from comment #4)
> Oh and computed goto has a similar issue too (PR 37722 which is for C++
> deconstructors but cleanup is the same idea there).

Right, that came up in our discussions, too.  I think the difference may be
that with `asm goto` we have the list of labels of possible jump targets and
thus possible edges. With computed goto ("indirect goto") we don't.  So I think
it makes sense to not run destructors for computed goto, and perhaps the
documentation should be made more explicit about that case (then PR 37722
closed as "Working as intended").

(In reply to John McCall from comment #5)
> > If this gets changed in GCC, I'd be happy to modify clang to match that updated behavior.
> 
> Policy-wise, I don't think clang would accept a patch making this UB
> (effectively what not calling the destructor/cleanup means) instead of
> ill-formed unless a standards body forced us to.  Not calling the
> destructor/cleanup seems like clearly undesirable behavior, and if we can
> define that away in the compiler with relative ease, we should.

Let me clarify.  If GCC were change behavior of `asm goto` to invoke the
destructor/cleanup function before the backwards edge of `asm goto`, I would
submit a patch to clang to implement that behavior as well.  I think we're
(John and I) in agreement that the destructors should be run.

[Bug middle-end/110728] should __attribute__((cleanup())) callback get invoked for indirect edges of asm goto

[rjmccall at gmail dot com]

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

--- Comment #8 from John McCall <rjmccall at gmail dot com> ---
> Let me clarify.  If GCC were change behavior of `asm goto` to
> invoke the destructor/cleanup function before the backwards edge
> of `asm goto`, I would submit a patch to clang to implement that
> behavior as well.

Ah, thank you, I understand what you're saying now.  Yes, we would take that.

I think this test case might be a little more illuminating and separates out
the issue of the back-edge and the uninitialized variable:
https://godbolt.org/z/doG7414va

```
#include <malloc.h>

#define GOTO 1
#define COMPUTED_GOTO 2
#define ASM_GOTO 3

#define CASE ASM_GOTO

static void free_from(int **ptr) {
    free(*ptr);
}

int test() {
#if CASE == COMPUTED_GOTO
    void *lbl = &&label;
#endif
    if (1) {
        __attribute__((cleanup(free_from))) int *p = calloc(sizeof(int), 1);
#if CASE == GOTO
        goto label;
#elif CASE == COMPUTED_GOTO
        goto *lbl;
#elif CASE == ASM_GOTO
        asm goto("<<< asm goto %l0 >>>"::::label);
#endif
    }
    label:
    return 0;
}
```

GCC produces this output for CASE == ASM_GOTO (stripping the .LFB labels):

```
test:
        push    rbp
        mov     rbp, rsp
        sub     rsp, 16
        mov     esi, 1
        mov     edi, 4
        call    calloc
        mov     QWORD PTR [rbp-8], rax
        <<< asm goto .L3 >>>
        lea     rax, [rbp-8]
        mov     rdi, rax
        call    free_from
.L3:
        mov     eax, 0
        leave
        ret
``

It does something similar for CASE == COMPUTED_GOTO.  In both cases, the branch
avoids the cleanup with no diagnostic.  But it does the right thing with CASE
== GOTO.

[Bug middle-end/110728] should __attribute__((cleanup())) callback get invoked for indirect edges of asm goto

[matz at gcc dot gnu.org]

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

Michael Matz <matz at gcc dot gnu.org> changed:

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

--- Comment #9 from Michael Matz <matz at gcc dot gnu.org> ---
Just for completeness: I agree with Andrew that the initial code example in
comment #0 doesn't show any problem.  The edge from asmgoto to l0 doesn't cross
the scope of the variable, hence no cleanups should be run.  The cleanup
call that is there is from the edge that leaves the function scope before
return, and it's placed correctly.

But the example from comment #8 indeed shows a problem with asm-goto.  But it
may be impossible to fix.  That has to do with how we need to (possibly) split
critical edges, which changes label identity, which in turn might actually
be the thing that's required by the asmgoto.  Like in such situation:

----------
extern int printf (const char *, ...);
extern void doit(void *p);

void foo (int cond)
{
  {
    int x __attribute__((cleanup(doit)));
    if (cond)
      asm goto("# A %l0"::::out);
    else
      asm goto("# B %l0"::::out);
  }
  printf("...");
  out:
}
----------

The calls to doit need to be emitted on the two edges asm1->out and asm2->out,
but must _not_ be emitted on the fallthrough printf->out (of course, because
the cleanup was already done on the edge x-scope->printf).  The only
way to do that is by splitting those edges which introduces new labels:

{
  {
    int x;
    if (cond)
      asm goto("# A %l0"::::out1);
    else
      asm goto("# B %l0"::::out2);
    doit();
  }
  printf("...");
 out:
  return;
out1: doit(); goto out;
out2: doit(); goto out;
}

(In this easy example we could save ourself by realizing that the out1 and out2
code sequences are the same, and hence could get away with just emitting
one new label instead of two.  In general that's not possible.)

That's the CFG manipulation that needs happening.  The question is what
it does to the asmgoto: both now receive different labels, whereas before
they received the same.  If whatever the asm does relies on the identity of
these label then this will break things.

Now, asm gotos were specifically introduced to be able to jump to stuff,
not to capture the identity of labels/code-addresses (as is done for instance
in the linux kernel alternative mechanism), so it may be fine to do the edge
splitting and associated renaming of labels.  But that needs to be a conscious
decision.

[Bug middle-end/110728] should __attribute__((cleanup())) callback get invoked for indirect edges of asm goto

[ndesaulniers at google dot com]

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

--- Comment #10 from Nick Desaulniers <ndesaulniers at google dot com> ---
(In reply to Michael Matz from comment #9)
> That has to do with how we need to (possibly)
> split
> critical edges, which changes label identity, which in turn might actually
> be the thing that's required by the asmgoto.

(Addressing just this point for now.)

I don't think that's a problem for users of asm goto; GCC (and Clang) will
already split critical edges when using asm goto with outputs (thus creating
the case you describe where the label that the asm will jump to has been
synthesized and may differ from the final actual destination).

For example:

```c
void bar (void);

int foo (int y) {
    int x;

    if (y)
        goto end;

    asm goto ("# %1 %2":"=r"(x):"i"(&&end)::end);
    bar();
    return 0;
end:
    return x;
}
```

in the above, %1 may not equal %2.

I documented this behavior in clang
https://github.com/llvm/llvm-project/commit/329ef60f3e21fd6845e8e8b0da405cae7eb27267#diff-ec770381d76c859f5f572db789175fe44410a72608f58ad5dbb14335ba56eb97
which will be in the clang-17 release notes once clang-17 ships (currently in
-rc).

I also don't see that being a problem for the Linux kernel at the moment,
though they may need to consider that behavior.

[Bug middle-end/110728] should __attribute__((cleanup())) callback get invoked for indirect edges of asm goto

[matz at gcc dot gnu.org]

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

--- Comment #11 from Michael Matz <matz at gcc dot gnu.org> ---
(In reply to Michael Matz from comment #9)
> Just for completeness: I agree with Andrew that the initial code example in
> comment #0 doesn't show any problem.  The edge from asmgoto to l0 doesn't
> cross
> the scope of the variable, hence no cleanups should be run.  The cleanup
> call that is there is from the edge that leaves the function scope before
> return, and it's placed correctly.

I was reminded that this is incorrect.  Though it isn't documented that way
(AFAICS) the cleanup attribute itself create a scope, as we're using the
try/finally middle-end mechanisms to implement this attribute.  We can't change
that behaviour anymore of course, so that's how it has to be: jumping in front
of the decl of 'x' _is_ supposed to run the cleanup.

The initial example essentially boils down to:

void test4(void) {
l0:;
        try { /* implicit scope per instantiation of __cleanup__ variable */
                int x __attribute__((cleanup(test4cleanup)));
                asm goto("# %l0"::::l0); /* <-- leaves scope created by x */
        } finally {
                test4cleanup();
        }
}

[Bug middle-end/110728] should __attribute__((cleanup())) callback get invoked for indirect edges of asm goto

[rjmccall at gmail dot com]

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

--- Comment #12 from John McCall <rjmccall at gmail dot com> ---
While it's theoretically possible to split a computed-goto edge, in practice
you want to avoid doing so if you at all can, because the split-edge pattern
defeats the interpreter optimization that's the primary purpose for computed
goto to exist.  Users should take care to ensure that their jumps are from a
"neutral" scope instead.  That's why Clang chose to diagnose computed gotos
that potentially leave destructor/cleanup scopes instead of just making it
work, and I expect we would continue doing that even if GCC started doing
edge-splitting.

`asm goto` is different, in my opinion, and that edge could productively be
split.  Clang isn't doing that right now only because diagnosing was the
quickest path to correctness (as opposed to skipping the cleanup); I think
we're looking into splitting as the more long-term solution.