Respected community members, Kindly give me feedback on the proposed patch fix in the previous email. I am attaching it again. (See: 0001-Fix-AIX-build-break.patch). AIX internal CI build for GDB has been broken for some time now. Your feedback will help us fix it and get the build back to be successful. I appreciate your patience, feedback and apologise for any inconvenience. Have a nice day ahead. Thanks and regards, Aditya. From: Aditya Kamath1 Date: Wednesday, 24 January 2024 at 6:23 AM To: Tom Tromey Cc: Tom Tromey , gdb-patches@sourceware.org , Sangamesh Mallayya Subject: [EXTERNAL] RE: [PATCH] Fix AIX build break. Respected community members, Kindly give me feedback on the proposed patch fix in the previous email. I am attaching it again. (See: 0001-Fix-AIX-build-break. patch). Thank you for your feedback so far. I appreciate the same. Have a nice day ZjQcmQRYFpfptBannerStart This Message Is From an External Sender This message came from outside your organization. Report Suspicious ZjQcmQRYFpfptBannerEnd Respected community members, Kindly give me feedback on the proposed patch fix in the previous email. I am attaching it again. (See: 0001-Fix-AIX-build-break.patch). Thank you for your feedback so far. I appreciate the same. Have a nice day ahead. Thanks and regards, Aditya. ----------------------------------------------------- From: Aditya Kamath1 Date: Saturday, 20 January 2024 at 8:34 PM To: Tom Tromey Cc: Tom Tromey , gdb-patches@sourceware.org , Sangamesh Mallayya Subject: [EXTERNAL] RE: [PATCH] Fix AIX build break. Respected Tom and community members, Hi, Please find attached a patch. (See: 0001-Fix-AIX-build-break. patch). Thank you for your guidance and patience. I have renamed the class to scoped_. Let’s keep it the same as rest of GDB. >This is ZjQcmQRYFpfptBannerStart This Message Is From an External Sender This message came from outside your organization. Report Suspicious ZjQcmQRYFpfptBannerEnd Respected Tom and community members, Hi, Please find attached a patch. (See: 0001-Fix-AIX-build-break.patch). Thank you for your guidance and patience. I have renamed the class to scoped_. Let’s keep it the same as rest of GDB. >This is static in utils.c now, so it means that nothing else can affect it. >However this changes how the interception works -- it disables it. But, >the reason the interception is done is to avoid emitting warnings from >worker threads. I get this. I got the fact that my patch in my previous email was horribly wrong. Let me explain you the design in this patch. So in the code in the commit (https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=54b815ddb428944a70694e3767a0fadbdd9ca9ea) I see we are setting the deprecated_warning_hook pointer to issue_complaint function and this is the only place where I see the pointer is set. If this pointer is set then we use this function pointer and its required parameters to display the warning via wrap_warning_hook (). One fact is that in the commit deprecated_warning_hook is declared as an extern thread_local in defs.h, perhaps defined in top.c and used in three places complaints.c, utils.c and interps.c. extern declaration made is easy to use it in three files but caused this undefined symbol error issue. So we need to ensure that we do not use extern and at the same time the deprecated_warning_hook pointer is declared only once and used by all the three files based on whether the pointer is nullptr or set to a function address. Inorder to do the same, I used a new header file warning-header.h and the below lines in it to ensure we declare it only once. +#ifndef GDB_WARNING_HOOK +#define GDB_WARNING_HOOK 1 + +static void (*deprecated_warning_hook) (const char *, va_list); + +#endif This header file is now included all the three files namely utils.c, complaints.c and interps.c. The class scoped_restore_warning_hook defined in defs.h is incharge of setting and restoring the function pointer whenever required. This object of this class is declared in the complaint_interceptor and initialised in complaints.c. Now we will have the function pointer set. So any file or thread (since the g_complaint_interceptor pointer is thread_local) who is using this function pointer deprecated_warning_hook can comfortably recognise if this pointer is set or not and can execute. Also, the destructor will ensure the pointer is back to where its original value. The only thing is why we need a new header file. So, I was searching for a header file that is common amongst the three files and I found it was defs.h. But the moment I declared the function pointer in the same style as warning-header.h many files importing defs.h complained about unused variable deprecated_warning_hook. Do suggest me if you have better alternatives for the pointer declaration or we can use some other header file accessible by all the three files or any other way. I would love to understand and implement. So this is the whole idea. I am thankful for your guidance to help me so that we can fix this. It took me time to figure this. Let me know if I am still wrong somewhere. Kindly give me a feedback for the same. Have a nice day ahead. Thanks and regards, Aditya. From: Tom Tromey Date: Friday, 19 January 2024 at 9:55 PM To: Aditya Kamath1 Cc: Tom Tromey , gdb-patches@sourceware.org , Sangamesh Mallayya Subject: [EXTERNAL] Re: [PATCH] Fix AIX build break. > The only thing I am unsure of is the utils.c file where we also use > the deprecated_warning_hook. Should we use an object of > warning_hook_class there as well? I'll add some notes inline. I don't think this patch can really work. > From the experiments I conducted this is true. It is because > g_complaint_interceptor is a static thread_local variable and not used > anywhere else we do not see it as an unreferenced symbol in AIX. Don't be deceived by the 'static' keyword. C++ overloads this. This variable is in fact extern. > +static thread_local void (*deprecated_warning_hook) (const char *, va_list); This is defined in two places. This is confusing at best, but the one here (in complaints.c) means that warning_hook_class won't interoperate properly with the other global. This seems very weird. > +warning_hook_class::warning_hook_class (warning_hook_handler new_handler) I wouldn't put '_class' in a class name. It's more normal in gdb to prefix scoped things with "scoped_", but other choices are possible. > diff --git a/gdb/utils.c b/gdb/utils.c > index b326033e9ff..23530c2a41f 100644 > --- a/gdb/utils.c > +++ b/gdb/utils.c > @@ -82,6 +82,8 @@ > #include "pager.h" > #include "run-on-main-thread.h" > +static thread_local void (*deprecated_warning_hook) (const char *, va_list); > + This is static in utils.c now, so it means that nothing else can affect it. However this changes how the interception works -- it disables it. But, the reason the interception is done is to avoid emitting warnings from worker threads. Tom