Re: [PATCH] Fix compile error due to [[noreturn]] with clang

Guinevere Larsen
2024-10-22T19:10:55Z

[-- Attachment #1: Type: text/plain, Size: 4258 bytes --]

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 */
> >
>

--
Cheers,
Guinevere Larsen
She/Her/Hers

[-- Attachment #2: Type: text/html, Size: 7496 bytes --]