| Message ID | DBBPR83MB06131CC027559613C5860339F8922@DBBPR83MB0613.EURPRD83.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | SMALL code model fixes, optimization fixes, LTO and minimal C++ enablement | expand |
On Mon, 2 Sep 2024, Evgeny Karpov wrote: > The patch adds support for weak references. The original MinGW > implementation targets ix86, which handles weak symbols differently > compared to AArch64. Please clarify this statement. There is no difference between architectures with respect to how weak symbols work in COFF objects - this is how it works in LLD for MinGW targets. If there are limitations in the implementation of weak symbols for x86 COFF object files in Binutils, then I would recommend unifying it, and fixing whichever architecture handles them incorrectly. Thus - please clarify whether you claim that this is a spec issue, that weak symbols are supposed to behave differently, or whether it is an implementation issue with Binutils that you are working around. // Martin
Monday, September 2, 2024 Richard Sandiford <richard.sandiford@arm.com> wrote: > On patch 1, do you have a reference for how AArch64 and x86 handle weak > references for MinGW? The code looks good, but I didn't really follow > why it was doing what it was doing. Monday, September 2, 2024 Martin Storsjö <martin@martin.st> >> The patch adds support for weak references. The original MinGW >> implementation targets ix86, which handles weak symbols differently >> compared to AArch64. > > Please clarify this statement. Here is an explanation of why this change is needed and what the difference is between x86_64-w64-mingw32 and aarch64-w64-mingw32. The way x86_64 calls a weak function: call weak_fn2 GCC emits the call and creates the required definitions at the end of the assembly: .weak weak_fn2 .def weak_fn2; .scl 2; .type 32; .endef This is different from aarch64: weak_fn2 will be legitimized and replaced by .refptr.weak_fn2, and there will be no other references to weak_fn2 in the code. adrp x0, .refptr.weak_fn2 add x0, x0, :lo12:.refptr.weak_fn2 ldr x0, [x0] blr x0 GCC does not emit the required definitions at the end of the assembly, and weak_fn2 is tracked only by the mingw stub sybmol. Without the change, the stub definition will emit: .section .rdata$.refptr.weak_fn2, "dr" .globl .refptr.weak_fn2 .linkonce discard .refptr.weak_fn2: .quad weak_fn2 which is not enough. This fix will emit the required definitions: .weak weak_fn2 .def weak_fn2; .scl 2; .type 32; .endef .section .rdata$.refptr.weak_fn2, "dr" .globl .refptr.weak_fn2 .linkonce discard .refptr.weak_fn2: .quad weak_fn2 Regards, Evgeny
On Thu, 5 Sep 2024, Evgeny Karpov wrote: > Monday, September 2, 2024 > Martin Storsjö <martin@martin.st> >>> The patch adds support for weak references. The original MinGW >>> implementation targets ix86, which handles weak symbols differently >>> compared to AArch64. >> >> Please clarify this statement. > > Here is an explanation of why this change is needed and what the > difference is between x86_64-w64-mingw32 and aarch64-w64-mingw32. > > The way x86_64 calls a weak function: > call weak_fn2 > > GCC emits the call and creates the required definitions at the end > of the assembly: > > .weak weak_fn2 > .def weak_fn2; .scl 2; .type 32; .endef > > This is different from aarch64: > > weak_fn2 will be legitimized and replaced by .refptr.weak_fn2, > and there will be no other references to weak_fn2 in the code. > > adrp x0, .refptr.weak_fn2 > add x0, x0, :lo12:.refptr.weak_fn2 > ldr x0, [x0] > blr x0 Right, this is the core of what I'm arguing here. Is there any intrinsic reason why there _should_ be a difference to x86_64 here? Because most of the same reasons for why aarch64 wants to do indirection via .refptr here also do apply for x86_64. Or to put it in more clear words: I think x86_64 also should use .refptr for weak symbols. There are a number of open bugs for GCC targeting x86_64 mingw, regarding weak symbols, and I think a few of them could be solved if GCC would use .refptr for the weak symbols on x86_64 as well. So I don't argue that this change is wrong, it probably is correct. But I'm arguing that there shouldn't be any difference between the architectures regarding how it is handled. Whenever there's an architecture difference in such a matter which shouldn't be architecture specific, there may be a latent bug hiding. That's not necessarily a blocker for this patch though, but the wording should make it clear: There's no specific reason for why aarch64 should behave differently than x86_64, but the x86_64 implementation probably needs to catch up. // Martin
diff --git a/gcc/config/aarch64/cygming.h b/gcc/config/aarch64/cygming.h index 9ce140a356f..bd6078023e3 100644 --- a/gcc/config/aarch64/cygming.h +++ b/gcc/config/aarch64/cygming.h @@ -171,7 +171,8 @@ still needed for compilation. */ mingw_handle_selectany_attribute, NULL } #undef SUB_TARGET_RECORD_STUB -#define SUB_TARGET_RECORD_STUB mingw_pe_record_stub +#define SUB_TARGET_RECORD_STUB(NAME, DECL) mingw_pe_record_stub((NAME), \ + DECL_WEAK ((DECL))) #define SUPPORTS_ONE_ONLY 1 @@ -186,7 +187,8 @@ still needed for compilation. */ #undef GOT_ALIAS_SET #define GOT_ALIAS_SET mingw_GOT_alias_set () -#define PE_COFF_LEGITIMIZE_EXTERN_DECL 1 +#define PE_COFF_LEGITIMIZE_EXTERN_DECL(RTX) \ + (GET_CODE (RTX) == SYMBOL_REF && SYMBOL_REF_WEAK (RTX)) #define HAVE_64BIT_POINTERS 1 diff --git a/gcc/config/i386/cygming.h b/gcc/config/i386/cygming.h index 9c8c7e33cc2..1633017eff6 100644 --- a/gcc/config/i386/cygming.h +++ b/gcc/config/i386/cygming.h @@ -461,7 +461,7 @@ do { \ #define TARGET_ASM_ASSEMBLE_VISIBILITY i386_pe_assemble_visibility #undef SUB_TARGET_RECORD_STUB -#define SUB_TARGET_RECORD_STUB mingw_pe_record_stub +#define SUB_TARGET_RECORD_STUB(NAME, DECL) mingw_pe_record_stub((NAME), 0) /* Static stack checking is supported by means of probes. */ #define STACK_CHECK_STATIC_BUILTIN 1 @@ -470,7 +470,7 @@ do { \ # define HAVE_GAS_ALIGNED_COMM 0 #endif -#define PE_COFF_LEGITIMIZE_EXTERN_DECL \ +#define PE_COFF_LEGITIMIZE_EXTERN_DECL(RTX) \ (ix86_cmodel == CM_LARGE_PIC || ix86_cmodel == CM_MEDIUM_PIC) #define HAVE_64BIT_POINTERS TARGET_64BIT_DEFAULT diff --git a/gcc/config/mingw/winnt-dll.cc b/gcc/config/mingw/winnt-dll.cc index f74495b7fda..eb7cff7a593 100644 --- a/gcc/config/mingw/winnt-dll.cc +++ b/gcc/config/mingw/winnt-dll.cc @@ -134,7 +134,7 @@ get_dllimport_decl (tree decl, bool beimport) { SYMBOL_REF_FLAGS (rtl) |= SYMBOL_FLAG_EXTERNAL; #ifdef SUB_TARGET_RECORD_STUB - SUB_TARGET_RECORD_STUB (name); + SUB_TARGET_RECORD_STUB (name, decl); #endif } @@ -206,7 +206,7 @@ legitimize_pe_coff_symbol (rtx addr, bool inreg) } } - if (!PE_COFF_LEGITIMIZE_EXTERN_DECL) + if (!PE_COFF_LEGITIMIZE_EXTERN_DECL (addr)) return NULL_RTX; if (GET_CODE (addr) == SYMBOL_REF diff --git a/gcc/config/mingw/winnt.cc b/gcc/config/mingw/winnt.cc index 803e5f5ec85..1e2ec53e841 100644 --- a/gcc/config/mingw/winnt.cc +++ b/gcc/config/mingw/winnt.cc @@ -635,6 +635,7 @@ struct GTY(()) stub_list { struct stub_list *next; const char *name; + bool is_weak_decl_needed; }; static GTY(()) struct export_list *export_head; @@ -672,7 +673,7 @@ mingw_pe_maybe_record_exported_symbol (tree decl, const char *name, int is_data) } void -mingw_pe_record_stub (const char *name) +mingw_pe_record_stub (const char *name, bool is_weak_decl_needed) { struct stub_list *p; @@ -691,6 +692,7 @@ mingw_pe_record_stub (const char *name) p = ggc_alloc<stub_list> (); p->next = stub_head; p->name = name; + p->is_weak_decl_needed = is_weak_decl_needed; stub_head = p; } @@ -807,6 +809,15 @@ mingw_pe_file_end (void) if (!startswith (name, "refptr.")) continue; name += 7; + + if (q->is_weak_decl_needed) + { +#ifdef ASM_WEAKEN_LABEL + ASM_WEAKEN_LABEL (asm_out_file, name); +#endif + mingw_pe_declare_function_type (asm_out_file, name, 1); + } + fprintf (asm_out_file, "\t.section\t.rdata$%s, \"dr\"\n" "\t.globl\t%s\n" "\t.linkonce\tdiscard\n", oname, oname); diff --git a/gcc/config/mingw/winnt.h b/gcc/config/mingw/winnt.h index 97fefbcebca..a21a36b7e5d 100644 --- a/gcc/config/mingw/winnt.h +++ b/gcc/config/mingw/winnt.h @@ -28,7 +28,7 @@ extern void mingw_pe_declare_function_type (FILE *file, const char *name, extern void mingw_pe_encode_section_info (tree, rtx, int); extern void mingw_pe_file_end (void); extern void mingw_pe_maybe_record_exported_symbol (tree, const char *, int); -extern void mingw_pe_record_stub (const char *); +extern void mingw_pe_record_stub (const char *, bool); extern unsigned int mingw_pe_section_type_flags (tree, const char *, int); extern void mingw_pe_unique_section (tree, int); extern bool mingw_pe_valid_dllimport_attribute_p (const_tree);