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