Re: Restore default 'sorry' 'TARGET_ASM_CONSTRUCTOR', 'TARGET_ASM_DESTRUCTOR' (was: [PATCH 1/3] STABS: remove -gstabs and -gxcoff functionality)

["Martin Liška" <mliska@suse.cz>]

On 10/10/22 16:19, Thomas Schwinge wrote:
> Hi!
> 
> On 2022-09-01T12:05:23+0200, Martin Liška <mliska@suse.cz> wrote:
>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>
>> I've also built all cross compilers.
> 
> First: thanks for that: clean up plus "built all cross compilers"!
> 
> But yet, I've now tracked down an issue related to these changes,
> apparently only visible via the nvptx back end -- and quite
> non-obvious...  ;-)
> 
>> --- a/gcc/config/nvptx/nvptx.cc
>> +++ b/gcc/config/nvptx/nvptx.cc
>> @@ -52,7 +52,6 @@
>>  #include "tm-preds.h"
>>  #include "tm-constrs.h"
>>  #include "langhooks.h"
>> -#include "dbxout.h"
>>  #include "cfgrtl.h"
>>  #include "gimple.h"
>>  #include "stor-layout.h"
> 
>> --- a/gcc/dbxout.cc
>> +++ /dev/null
>> @@ -1,3936 +0,0 @@
>> -/* Output dbx-format symbol table information from GNU compiler.
> 
> The "dbx-format symbol table information" stuff indeed is not relevant
> anymore, but:
> 
>> -/* Record an element in the table of global destructors.  SYMBOL is
>> -   a SYMBOL_REF of the function to be called; PRIORITY is a number
>> -   between 0 and MAX_INIT_PRIORITY.  */
>> -
>> -void
>> -default_stabs_asm_out_destructor (rtx symbol ATTRIBUTE_UNUSED,
>> -                               int priority ATTRIBUTE_UNUSED)
>> -{
>> -#if defined DBX_DEBUGGING_INFO || defined XCOFF_DEBUGGING_INFO
>> -  /* Tell GNU LD that this is part of the static destructor set.
>> -     This will work for any system that uses stabs, most usefully
>> -     aout systems.  */
>> -  dbxout_begin_simple_stabs ("___DTOR_LIST__", 22 /* N_SETT */);
>> -  dbxout_stab_value_label (XSTR (symbol, 0));
>> -#else
>> -  sorry ("global destructors not supported on this target");
>> -#endif
>> -}
>> -
>> -/* Likewise for global constructors.  */
>> -
>> -void
>> -default_stabs_asm_out_constructor (rtx symbol ATTRIBUTE_UNUSED,
>> -                                int priority ATTRIBUTE_UNUSED)
>> -{
>> -#if defined DBX_DEBUGGING_INFO || defined XCOFF_DEBUGGING_INFO
>> -  /* Tell GNU LD that this is part of the static destructor set.
>> -     This will work for any system that uses stabs, most usefully
>> -     aout systems.  */
>> -  dbxout_begin_simple_stabs ("___CTOR_LIST__", 22 /* N_SETT */);
>> -  dbxout_stab_value_label (XSTR (symbol, 0));
>> -#else
>> -  sorry ("global constructors not supported on this target");
>> -#endif
>> -}
> 
>> --- a/gcc/dbxout.h
>> +++ /dev/null
>> @@ -1,60 +0,0 @@
>> -/* dbxout.h - Various declarations for functions found in dbxout.cc
> 
>> -extern void default_stabs_asm_out_destructor (rtx, int);
>> -extern void default_stabs_asm_out_constructor (rtx, int);
> 
> ... these two functions, 'default_stabs_asm_out_constructor',
> 'default_stabs_asm_out_destructor' (specifically, now their 'sorry'
> branches only) used to serve as default 'TARGET_ASM_CONSTRUCTOR',
> 'TARGET_ASM_DESTRUCTOR' via...
> 
>> --- a/gcc/target-def.h
>> +++ b/gcc/target-def.h
> 
> |  #if !defined(TARGET_ASM_CONSTRUCTOR) && !defined(USE_COLLECT2)
> |  # ifdef CTORS_SECTION_ASM_OP
> |  #  define TARGET_ASM_CONSTRUCTOR default_ctor_section_asm_out_constructor
>>  # else
>>  #  ifdef TARGET_ASM_NAMED_SECTION
>>  #   define TARGET_ASM_CONSTRUCTOR default_named_section_asm_out_constructor
>> -#  else
>> -#   define TARGET_ASM_CONSTRUCTOR default_stabs_asm_out_constructor
>>  #  endif
>>  # endif
>>  #endif
>> @@ -74,8 +72,6 @@
> |  #if !defined(TARGET_ASM_DESTRUCTOR) && !defined(USE_COLLECT2)
> |  # ifdef DTORS_SECTION_ASM_OP
> |  #  define TARGET_ASM_DESTRUCTOR default_dtor_section_asm_out_destructor
>>  # else
>>  #  ifdef TARGET_ASM_NAMED_SECTION
>>  #   define TARGET_ASM_DESTRUCTOR default_named_section_asm_out_destructor
>> -#  else
>> -#   define TARGET_ASM_DESTRUCTOR default_stabs_asm_out_destructor
>>  #  endif
>>  # endif
>>  #endif
> 
> ... this setup here (manually added some more context to the 'diff').
> 
> That is, if a back end was not 'USE_COLLECT2', nor manually defined
> 'TARGET_ASM_CONSTRUCTOR', 'TARGET_ASM_DESTRUCTOR', or got pointed to the
> respective 'default_[...]'  functions due to 'CTORS_SECTION_ASM_OP',
> 'DTORS_SECTION_ASM_OP', or 'TARGET_ASM_NAMED_SECTION', it got pointed to
> 'default_stabs_asm_out_constructor', 'default_stabs_asm_out_destructor'.
> These would emit 'sorry' for any global constructor/destructor they're
> run into.
> 
> This is now gone, and thus in such a back end configuration case
> 'TARGET_ASM_CONSTRUCTOR', 'TARGET_ASM_DESTRUCTOR' don't get defined
> anymore, and thus the subsequently following:
> 
>     #if !defined(TARGET_HAVE_CTORS_DTORS)
>     # if defined(TARGET_ASM_CONSTRUCTOR) && defined(TARGET_ASM_DESTRUCTOR)
>     # define TARGET_HAVE_CTORS_DTORS true
>     # endif
>     #endif
> 
> ... doesn't define 'TARGET_HAVE_CTORS_DTORS' anymore, and thus per my
> understanding, 'gcc/final.cc:rest_of_handle_final':
> 
>     if (DECL_STATIC_CONSTRUCTOR (current_function_decl)
>         && targetm.have_ctors_dtors)
>       targetm.asm_out.constructor (XEXP (DECL_RTL (current_function_decl), 0),
>                                    decl_init_priority_lookup
>                                      (current_function_decl));
>     if (DECL_STATIC_DESTRUCTOR (current_function_decl)
>         && targetm.have_ctors_dtors)
>       targetm.asm_out.destructor (XEXP (DECL_RTL (current_function_decl), 0),
>                                   decl_fini_priority_lookup
>                                     (current_function_decl));
> 
> ... simply does nothing anymore for a 'DECL_STATIC_CONSTRUCTOR',
> 'DECL_STATIC_DESTRUCTOR'.
> 
> This, effectively, means that GCC/nvptx now suddenly appears to "support"
> global constructors/destructors, which means that a ton of test cases now
> erroneously PASS that previously used to FAIL:
> 
>     sorry, unimplemented: global constructors not supported on this target
> 
> Of course, such support didn't magically happen due to
> "STABS: remove -gstabs and -gxcoff functionality", so this is bad.  And,
> corresponding execution testing then regularly FAILs (due to the global
> constructor/destructor functions never being invoked), for example:
> 
>     [-UNSUPPORTED:-]{+PASS:+} gcc.dg/initpri1.c {+(test for excess errors)+}
>     {+FAIL: gcc.dg/initpri1.c execution test+}
> 
>     [-UNSUPPORTED:-]{+PASS:+} g++.dg/special/conpr-1.C {+(test for excess errors)+}
>     {+FAIL: g++.dg/special/conpr-1.C execution test+}
> 
> To restore the previous GCC/nvptx behavior, I seek permission to first
> push the attached
> "Restore default 'sorry' 'TARGET_ASM_CONSTRUCTOR', 'TARGET_ASM_DESTRUCTOR'".
> For traceability, this simply restores the previous code, stripped down
> to the bare minimum.
> 
> As a next step, I'd then prepare a patch to re-work this thing, to make
> it more obvious what's actually happening there.  (Details to be done.
> For example, do we want to continue to have 'sorry' functions called via
> 'gcc/target-def.h', as done previously?  Or, should that be an '#error'
> case there, and the nvptx back end provides its own 'sorry' functions for
> 'TARGET_ASM_CONSTRUCTOR', 'TARGET_ASM_DESTRUCTOR'?  Or, emit 'sorry' in
> 'gcc/final.cc:rest_of_handle_final'?  Or, something else?)
> 
> 
> In addition to 'gcc/config/nvptx/nvptx.cc' (as quoted above) three more
> files did '#include "dbxout.h"', and thus potentially could use these
> 'sorry' default functions.
> 
> Per my testing of '--target=pdp11-aout' as well as
> '--target=powerpc64-darwin', re-introducing '#include "dbxout.h"' is not
> relevant for 'gcc/config/pdp11/pdp11.cc' as well as for
> 'gcc/config/rs6000/rs6000-call.cc' and
> 'gcc/config/rs6000/rs6000-logue.cc' (for 'TARGET_MACHO' only).
> 
> Target pdp11-aout is not affected, because of 'gcc/config.gcc' setting:
> 
>     # On a.out targets, we need to use collect2.
>     case ${target} in
>     *-*-*aout*)
>             use_collect2=yes
>             ;;
>     esac
> 
> Target powerpc64-darwin is not affected, because it defines
> 'TARGET_ASM_NAMED_SECTION'.
> 
> I'll thus remove those changes/TODO markers that are still present in the
> attached
> "Restore default 'sorry' 'TARGET_ASM_CONSTRUCTOR', 'TARGET_ASM_DESTRUCTOR'".
> With that, OK to push?
> 
> 
> Grüße
>  Thomas
> 
> 
> -----------------
> Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

Hi Thomas.

Thanks for the fix, really appreciated!

Martin