Fix compile error due to [[noreturn]] with clang

  From: Andrew Oates <andrew@andrewoates.com>

Since commit d9deb60b2e9e94b532f43a7d3ddddf5ddf6dbdd3, I get the
following compiler error when building binutils (cross-compiling) on
macos:

 CXX    remote-sim.o
../../gdb/remote-sim.c:334:28: error: assigning to 'void (*)(host_callback *, const char *, ...) __attribute__((noreturn))' (aka 'void (*)(host_callback_struct *, const char *, ...) __attribute__((noreturn))') from incompatible type 'void (host_callback
*, const char *, ...)' (aka 'void (host_callback_struct *, const char *, ...)')
      gdb_callback.error = gdb_os_error;
                           ^~~~~~~~~~~~
1 error generated.

This appears to be due to the mismatch between ATTRIBUTE_NORETURN and
[[noreturn]] on gdb_os_error.  Removing ATTTRIBUTE_NORETURN on the
declaration of host_callback::error resolves the issue.

Tested by compiling on macos both with the system clang, as well as with
GCC 14.  With clang, remote-sim.c does not compile (per above) without
this patch.  With GCC, it compiles with and without the patch (it
doesn't link, but AFAICT that is unrelated).
---
 include/sim/callback.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

On 10/20/24 3:00 PM, andrew@andrewoates.com wrote:
> From: Andrew Oates <andrew@andrewoates.com>
>
> Since commit d9deb60b2e9e94b532f43a7d3ddddf5ddf6dbdd3, I get the
> following compiler error when building binutils (cross-compiling) on
> macos:
>
>   CXX    remote-sim.o
> ../../gdb/remote-sim.c:334:28: error: assigning to 'void (*)(host_callback *, const char *, ...) __attribute__((noreturn))' (aka 'void (*)(host_callback_struct *, const char *, ...) __attribute__((noreturn))') from incompatible type 'void (host_callback
> *, const char *, ...)' (aka 'void (host_callback_struct *, const char *, ...)')
>        gdb_callback.error = gdb_os_error;
>                             ^~~~~~~~~~~~
> 1 error generated.
>
> This appears to be due to the mismatch between ATTRIBUTE_NORETURN and
> [[noreturn]] on gdb_os_error.  Removing ATTTRIBUTE_NORETURN on the
> declaration of host_callback::error resolves the issue.

Have you tried using ATTRIBUTE_NORETURN for gdb_os_error instead? If the 
problem is the mismatch, I would prefer that we made them match over 
removing information for the compiler.

Also, from my little knowledge in this area, this sounds like a clang 
bug. I encourage you to report it to upstream clang, or I can do it 
myself if you'd prefer.
  

On Tue, Oct 22, 2024 at 2:29 PM Guinevere Larsen <guinevere@redhat.com>
wrote:

> On 10/20/24 3:00 PM, andrew@andrewoates.com wrote:
> > From: Andrew Oates <andrew@andrewoates.com>
> >
> > Since commit d9deb60b2e9e94b532f43a7d3ddddf5ddf6dbdd3, I get the
> > following compiler error when building binutils (cross-compiling) on
> > macos:
> >
> >   CXX    remote-sim.o
> > ../../gdb/remote-sim.c:334:28: error: assigning to 'void
> (*)(host_callback *, const char *, ...) __attribute__((noreturn))' (aka
> 'void (*)(host_callback_struct *, const char *, ...)
> __attribute__((noreturn))') from incompatible type 'void (host_callback
> > *, const char *, ...)' (aka 'void (host_callback_struct *, const char *,
> ...)')
> >        gdb_callback.error = gdb_os_error;
> >                             ^~~~~~~~~~~~
> > 1 error generated.
> >
> > This appears to be due to the mismatch between ATTRIBUTE_NORETURN and
> > [[noreturn]] on gdb_os_error.  Removing ATTTRIBUTE_NORETURN on the
> > declaration of host_callback::error resolves the issue.
>
> Have you tried using ATTRIBUTE_NORETURN for gdb_os_error instead? If the
> problem is the mismatch, I would prefer that we made them match over
> removing information for the compiler.
>

gdb_os_error used to have ATTRIBUTE_NORETURN on it, but that was removed in
favor of [[noreturn]] in commit d9deb60b2e9e94b532f43a7d3ddddf5ddf6dbdd3
(which seems to have done ATTRIBUTE_NORETURN -> [[noreturn]] through most
of the codebase).

I'm definitely not an expert here, but I agree that if there's a way to
annotate the function pointer instead so it can be assigned to a
[[noreturn]] function, that would be better.  I tried doing that but
couldn't make it work.


>
> Also, from my little knowledge in this area, this sounds like a clang
> bug. I encourage you to report it to upstream clang, or I can do it
> myself if you'd prefer.
>

Ah...maybe.  That didn't occur to me, but you could be right, if clang
should be recognizing __attribute__((noreturn)) as equivalent to
[[noreturn]].


>
> --
> Cheers,
> Guinevere Larsen
> She/Her/Hers
>
> >
> > Tested by compiling on macos both with the system clang, as well as with
> > GCC 14.  With clang, remote-sim.c does not compile (per above) without
> > this patch.  With GCC, it compiles with and without the patch (it
> > doesn't link, but AFAICT that is unrelated).
> > ---
> >   include/sim/callback.h | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/sim/callback.h b/include/sim/callback.h
> > index f69f783abac..045ac3411af 100644
> > --- a/include/sim/callback.h
> > +++ b/include/sim/callback.h
> > @@ -129,7 +129,7 @@ struct host_callback_struct
> >        In the case of gdb "exiting" means doing a longjmp back to the
> main
> >        command loop.  */
> >     void (*error) (host_callback *, const char *, ...)
> > -    ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF_2;
> > +    ATTRIBUTE_PRINTF_2;
> >
> >     int last_errno;           /* host format */
> >
>
>
  

On 10/22/24 3:50 PM, Andrew Oates wrote:
>
>
> On Tue, Oct 22, 2024 at 2:29 PM Guinevere Larsen 
> <guinevere@redhat.com> wrote:
>
>     On 10/20/24 3:00 PM, andrew@andrewoates.com wrote:
>     > From: Andrew Oates <andrew@andrewoates.com>
>     >
>     > Since commit d9deb60b2e9e94b532f43a7d3ddddf5ddf6dbdd3, I get the
>     > following compiler error when building binutils (cross-compiling) on
>     > macos:
>     >
>     >   CXX    remote-sim.o
>     > ../../gdb/remote-sim.c:334:28: error: assigning to 'void
>     (*)(host_callback *, const char *, ...) __attribute__((noreturn))'
>     (aka 'void (*)(host_callback_struct *, const char *, ...)
>     __attribute__((noreturn))') from incompatible type 'void
>     (host_callback
>     > *, const char *, ...)' (aka 'void (host_callback_struct *, const
>     char *, ...)')
>     >        gdb_callback.error = gdb_os_error;
>     >                             ^~~~~~~~~~~~
>     > 1 error generated.
>     >
>     > This appears to be due to the mismatch between
>     ATTRIBUTE_NORETURN and
>     > [[noreturn]] on gdb_os_error.  Removing ATTTRIBUTE_NORETURN on the
>     > declaration of host_callback::error resolves the issue.
>
>     Have you tried using ATTRIBUTE_NORETURN for gdb_os_error instead?
>     If the
>     problem is the mismatch, I would prefer that we made them match over
>     removing information for the compiler.
>
>
> gdb_os_error used to have ATTRIBUTE_NORETURN on it, but that was 
> removed in favor of [[noreturn]] in 
> commit d9deb60b2e9e94b532f43a7d3ddddf5ddf6dbdd3 (which seems to have 
> done ATTRIBUTE_NORETURN -> [[noreturn]] through most of the codebase).
>
> I'm definitely not an expert here, but I agree that if there's a way 
> to annotate the function pointer instead so it can be assigned to a 
> [[noreturn]] function, that would be better.  I tried doing that but 
> couldn't make it work.

Yeah, [[noreturn]] only works for functions, not meant to be used by 
members or variables, so this wouldn't work. We have to walk back the 
[[noreturn]] change for this function if we want to continue adding this 
information for the compiler.

I added Simon on CC since he's the one who made the original commit you 
pointed to. My reading seems to be that the patch is meant to modernize 
the code and isn't driven by an actual need, so I would think walking 
back this specific change should be ok, but I'll defer to Simon on this.

>
>     Also, from my little knowledge in this area, this sounds like a clang
>     bug. I encourage you to report it to upstream clang, or I can do it
>     myself if you'd prefer.
>
>
> Ah...maybe.  That didn't occur to me, but you could be right, if clang 
> should be recognizing __attribute__((noreturn)) as equivalent to 
> [[noreturn]].
Yeah, the way I'm reading the error messages, it seems to me that clang 
is ignoring [[noreturn]] completely, and gcc engineers agreed this 
should work, so I am confident the bug is at least worth filing. Worst 
case scenario we'll learn the logic behind clang and we can document in 
the code.
>
>
>     -- 
>     Cheers,
>     Guinevere Larsen
>     She/Her/Hers
>
>     >
>     > Tested by compiling on macos both with the system clang, as well
>     as with
>     > GCC 14.  With clang, remote-sim.c does not compile (per above)
>     without
>     > this patch.  With GCC, it compiles with and without the patch (it
>     > doesn't link, but AFAICT that is unrelated).
>     > ---
>     >   include/sim/callback.h | 2 +-
>     >   1 file changed, 1 insertion(+), 1 deletion(-)
>     >
>     > diff --git a/include/sim/callback.h b/include/sim/callback.h
>     > index f69f783abac..045ac3411af 100644
>     > --- a/include/sim/callback.h
>     > +++ b/include/sim/callback.h
>     > @@ -129,7 +129,7 @@ struct host_callback_struct
>     >        In the case of gdb "exiting" means doing a longjmp back
>     to the main
>     >        command loop.  */
>     >     void (*error) (host_callback *, const char *, ...)
>     > -    ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF_2;
>     > +    ATTRIBUTE_PRINTF_2;
>     >
>     >     int last_errno;           /* host format */
>     >
>
  

On Tue, Oct 22, 2024 at 3:10 PM Guinevere Larsen <guinevere@redhat.com>
wrote:

> On 10/22/24 3:50 PM, Andrew Oates wrote:
>
>
>
> On Tue, Oct 22, 2024 at 2:29 PM Guinevere Larsen <guinevere@redhat.com>
> wrote:
>
>> On 10/20/24 3:00 PM, andrew@andrewoates.com wrote:
>> > From: Andrew Oates <andrew@andrewoates.com>
>> >
>> > Since commit d9deb60b2e9e94b532f43a7d3ddddf5ddf6dbdd3, I get the
>> > following compiler error when building binutils (cross-compiling) on
>> > macos:
>> >
>> >   CXX    remote-sim.o
>> > ../../gdb/remote-sim.c:334:28: error: assigning to 'void
>> (*)(host_callback *, const char *, ...) __attribute__((noreturn))' (aka
>> 'void (*)(host_callback_struct *, const char *, ...)
>> __attribute__((noreturn))') from incompatible type 'void (host_callback
>> > *, const char *, ...)' (aka 'void (host_callback_struct *, const char
>> *, ...)')
>> >        gdb_callback.error = gdb_os_error;
>> >                             ^~~~~~~~~~~~
>> > 1 error generated.
>> >
>> > This appears to be due to the mismatch between ATTRIBUTE_NORETURN and
>> > [[noreturn]] on gdb_os_error.  Removing ATTTRIBUTE_NORETURN on the
>> > declaration of host_callback::error resolves the issue.
>>
>> Have you tried using ATTRIBUTE_NORETURN for gdb_os_error instead? If the
>> problem is the mismatch, I would prefer that we made them match over
>> removing information for the compiler.
>>
>
> gdb_os_error used to have ATTRIBUTE_NORETURN on it, but that was removed
> in favor of [[noreturn]] in commit d9deb60b2e9e94b532f43a7d3ddddf5ddf6dbdd3
> (which seems to have done ATTRIBUTE_NORETURN -> [[noreturn]] through most
> of the codebase).
>
> I'm definitely not an expert here, but I agree that if there's a way to
> annotate the function pointer instead so it can be assigned to a
> [[noreturn]] function, that would be better.  I tried doing that but
> couldn't make it work.
>
> Yeah, [[noreturn]] only works for functions, not meant to be used by
> members or variables, so this wouldn't work. We have to walk back the
> [[noreturn]] change for this function if we want to continue adding this
> information for the compiler.
>
> I added Simon on CC since he's the one who made the original commit you
> pointed to. My reading seems to be that the patch is meant to modernize the
> code and isn't driven by an actual need, so I would think walking back this
> specific change should be ok, but I'll defer to Simon on this.
>
>
>
>>
>> Also, from my little knowledge in this area, this sounds like a clang
>> bug. I encourage you to report it to upstream clang, or I can do it
>> myself if you'd prefer.
>>
>
> Ah...maybe.  That didn't occur to me, but you could be right, if clang
> should be recognizing __attribute__((noreturn)) as equivalent to
> [[noreturn]].
>
> Yeah, the way I'm reading the error messages, it seems to me that clang is
> ignoring [[noreturn]] completely, and gcc engineers agreed this should
> work, so I am confident the bug is at least worth filing. Worst case
> scenario we'll learn the logic behind clang and we can document in the code.
>

Makes sense!  I'll file the bug against clang and CC you and we'll see
where it goes :)


>
>
>>
>> --
>> Cheers,
>> Guinevere Larsen
>> She/Her/Hers
>>
>> >
>> > Tested by compiling on macos both with the system clang, as well as with
>> > GCC 14.  With clang, remote-sim.c does not compile (per above) without
>> > this patch.  With GCC, it compiles with and without the patch (it
>> > doesn't link, but AFAICT that is unrelated).
>> > ---
>> >   include/sim/callback.h | 2 +-
>> >   1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/include/sim/callback.h b/include/sim/callback.h
>> > index f69f783abac..045ac3411af 100644
>> > --- a/include/sim/callback.h
>> > +++ b/include/sim/callback.h
>> > @@ -129,7 +129,7 @@ struct host_callback_struct
>> >        In the case of gdb "exiting" means doing a longjmp back to the
>> main
>> >        command loop.  */
>> >     void (*error) (host_callback *, const char *, ...)
>> > -    ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF_2;
>> > +    ATTRIBUTE_PRINTF_2;
>> >
>> >     int last_errno;           /* host format */
>> >
>>
>>
> --
> Cheers,
> Guinevere Larsen
> She/Her/Hers
>
>
  

On 2024-10-22 15:10, Guinevere Larsen wrote:
> On 10/22/24 3:50 PM, Andrew Oates wrote:
>>
>>
>> On Tue, Oct 22, 2024 at 2:29 PM Guinevere Larsen <guinevere@redhat.com> wrote:
>>
>>     On 10/20/24 3:00 PM, andrew@andrewoates.com wrote:
>>     > From: Andrew Oates <andrew@andrewoates.com>
>>     >
>>     > Since commit d9deb60b2e9e94b532f43a7d3ddddf5ddf6dbdd3, I get the
>>     > following compiler error when building binutils (cross-compiling) on
>>     > macos:
>>     >
>>     >   CXX    remote-sim.o
>>     > ../../gdb/remote-sim.c:334:28: error: assigning to 'void (*)(host_callback *, const char *, ...) __attribute__((noreturn))' (aka 'void (*)(host_callback_struct *, const char *, ...) __attribute__((noreturn))') from incompatible type 'void (host_callback
>>     > *, const char *, ...)' (aka 'void (host_callback_struct *, const char *, ...)')
>>     >        gdb_callback.error = gdb_os_error;
>>     >                             ^~~~~~~~~~~~
>>     > 1 error generated.
>>     >
>>     > This appears to be due to the mismatch between ATTRIBUTE_NORETURN and
>>     > [[noreturn]] on gdb_os_error.  Removing ATTTRIBUTE_NORETURN on the
>>     > declaration of host_callback::error resolves the issue.
>>
>>     Have you tried using ATTRIBUTE_NORETURN for gdb_os_error instead? If the
>>     problem is the mismatch, I would prefer that we made them match over
>>     removing information for the compiler.
>>
>>
>> gdb_os_error used to have ATTRIBUTE_NORETURN on it, but that was removed in favor of [[noreturn]] in commit d9deb60b2e9e94b532f43a7d3ddddf5ddf6dbdd3 (which seems to have done ATTRIBUTE_NORETURN -> [[noreturn]] through most of the codebase).
>>
>> I'm definitely not an expert here, but I agree that if there's a way to annotate the function pointer instead so it can be assigned to a [[noreturn]] function, that would be better.  I tried doing that but couldn't make it work.
> 
> Yeah, [[noreturn]] only works for functions, not meant to be used by members or variables, so this wouldn't work. We have to walk back the [[noreturn]] change for this function if we want to continue adding this information for the compiler.
> 
> I added Simon on CC since he's the one who made the original commit you pointed to. My reading seems to be that the patch is meant to modernize the code and isn't driven by an actual need, so I would think walking back this specific change should be ok, but I'll defer to Simon on this.

So, I can't reproduce the build failure when configuring with
`--enable-targets=all --enable-sim`.  It looks like remote-sim.o doesn't
get built, which is odd given that the simulator is enabled.  Sounds
like a bug?

When building GDB for the ARM target specifically, remote-sim.o does get
built and I see the failure.

Reverting to using the old attribute is fine with me, I don't see any
other way around.

Simon
  

On Okt 22 2024, Simon Marchi wrote:

> So, I can't reproduce the build failure when configuring with
> `--enable-targets=all --enable-sim`.  It looks like remote-sim.o doesn't
> get built, which is odd given that the simulator is enabled.  Sounds
> like a bug?

It's only enabled for the main gdb target.
  

On 2024-10-22 16:20, Andreas Schwab wrote:
> On Okt 22 2024, Simon Marchi wrote:
> 
>> So, I can't reproduce the build failure when configuring with
>> `--enable-targets=all --enable-sim`.  It looks like remote-sim.o doesn't
>> get built, which is odd given that the simulator is enabled.  Sounds
>> like a bug?
> 
> It's only enabled for the main gdb target.
> 

But nowadays, --enable-targets=all builds the sim for all targets (that
have a sim), not only the main target.  Would it be possible to build
remote-sim.o in that case, and use the simulator for any target, from a
single --enable-targets=all gdb build?

Simon
  

On Okt 22 2024, Simon Marchi wrote:

> On 2024-10-22 16:20, Andreas Schwab wrote:
>> On Okt 22 2024, Simon Marchi wrote:
>> 
>>> So, I can't reproduce the build failure when configuring with
>>> `--enable-targets=all --enable-sim`.  It looks like remote-sim.o doesn't
>>> get built, which is odd given that the simulator is enabled.  Sounds
>>> like a bug?
>> 
>> It's only enabled for the main gdb target.
>> 
>
> But nowadays, --enable-targets=all builds the sim for all targets (that
> have a sim), not only the main target.  Would it be possible to build
> remote-sim.o in that case, and use the simulator for any target, from a
> single --enable-targets=all gdb build?

There can only be one simulator linked into gdb.
  

Filed as https://github.com/llvm/llvm-project/issues/113511

Thanks everyone!

@Simon Marchi <simark@simark.ca> are you going to send a rollback, or
should I try and put that together?

On Tue, Oct 22, 2024 at 4:53 PM Andreas Schwab <schwab@linux-m68k.org>
wrote:

> On Okt 22 2024, Simon Marchi wrote:
>
> > On 2024-10-22 16:20, Andreas Schwab wrote:
> >> On Okt 22 2024, Simon Marchi wrote:
> >>
> >>> So, I can't reproduce the build failure when configuring with
> >>> `--enable-targets=all --enable-sim`.  It looks like remote-sim.o
> doesn't
> >>> get built, which is odd given that the simulator is enabled.  Sounds
> >>> like a bug?
> >>
> >> It's only enabled for the main gdb target.
> >>
> >
> > But nowadays, --enable-targets=all builds the sim for all targets (that
> > have a sim), not only the main target.  Would it be possible to build
> > remote-sim.o in that case, and use the simulator for any target, from a
> > single --enable-targets=all gdb build?
>
> There can only be one simulator linked into gdb.
>
> --
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
> "And now for something completely different."
>
  

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> But nowadays, --enable-targets=all builds the sim for all targets (that
Simon> have a sim), not only the main target.  Would it be possible to build
Simon> remote-sim.o in that case, and use the simulator for any target, from a
Simon> single --enable-targets=all gdb build?

Andreas pointed out why this doesn't work today, but I wanted to mention
that I think we should have a goal of removing remote-sim.o entirely.

In particular I think the the sim should run as a separate process and
talk to gdb via the remote protocol.  "target sim" would be
reimplemented as "target remote | arch-name-whatever".

This way, the sim could be made multi-target-capable and also wouldn't
block the goal of always using target async.

I started work on this a bit, turning gdbserver into a library and
writing a gdbserver target layer for the sim.  However, I'm a little
blocked on figuring out how to handle register descriptions.  There's
also the gdbarch "register_sim_regno" hook to remove.

IMO the sims are a bit of an albatross.  Mike did a bunch of work on
them a while ago, but for the most part they don't seem to be actively
developed.

Tom