On Tue, Oct 22, 2024 at 3:10 PM Guinevere Larsen wrote: > On 10/22/24 3:50 PM, Andrew Oates wrote: > > > > On Tue, Oct 22, 2024 at 2:29 PM Guinevere Larsen > wrote: > >> On 10/20/24 3:00 PM, andrew@andrewoates.com wrote: >> > From: Andrew Oates >> > >> > 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 > >