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

[Andrew Oates <andrew@andrewoates.com>]

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

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

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