From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id C136F3858CDB for ; Mon, 10 Jun 2024 17:02:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C136F3858CDB Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org C136F3858CDB Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=217.140.110.172 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1718038965; cv=none; b=IbXcl2B3lAn60642QQfRLv2xFDA6vQBRR3y19kNAt15FI47LHuhxn6hKaIYfwpKtS3utdkOP3O9N4WMs8jE1+lziDl0/6U0SzSbqs7Vo6gc0wlcriJBDdx1x+NNj/FFM9Hho8eNgqRwnZtNiuoKPczY/+GqG+L2jO0vQxGsomvs= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1718038965; c=relaxed/simple; bh=oYEqA0p10VUbnzRIQAJmqu9jSv0xpm/N9Un+buZwLwc=; h=From:To:Subject:Date:Message-ID:MIME-Version; b=Ou4AxThVpPFJQD5bCt0B/0r+cLm0QOaD9KccyDE0CAJe9w2TWlBVqx2mFWguNkRLb9LvlRqYcWMWF6cRXjICtMRNd3GQ2fShlxz3QJAnZYIADsYsHbiUiBXYcq6U1/doxIvfn1Cy4+5JteXxnAstcufH+tWEz6ykzO1NkAanP90= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 57F4F106F; Mon, 10 Jun 2024 10:02:58 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.110.72]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 2376C3F73B; Mon, 10 Jun 2024 10:02:33 -0700 (PDT) From: Richard Sandiford To: Evgeny Karpov Mail-Followup-To: Evgeny Karpov ,"gcc-patches\@gcc.gnu.org" , richard.sandiford@arm.com Cc: "gcc-patches\@gcc.gnu.org" Subject: Re: [PATCH v3 6/6] aarch64: Add DLL import/export to AArch64 target References: Date: Mon, 10 Jun 2024 18:02:31 +0100 In-Reply-To: (Evgeny Karpov's message of "Sat, 8 Jun 2024 13:49:17 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-19.3 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,KAM_STOCKGEN,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Thanks for the update. Parts 1-5 look good to me. Some minor comments below about part 6: Evgeny Karpov writes: > This patch reuses the MinGW implementation to enable DLL import/export > functionality for the aarch64-w64-mingw32 target. It also modifies > environment configurations for MinGW. > > gcc/ChangeLog: > > * config.gcc: Add winnt-dll.o, which contains the DLL > import/export implementation. > * config/aarch64/aarch64.cc (aarch64_legitimize_pe_coff_symbol): > Add a conditional function that reuses the MinGW implementation > for COFF and does nothing otherwise. > (aarch64_load_symref_appropriately): Add dllimport > implementation. > (aarch64_expand_call): Likewise. > (aarch64_legitimize_address): Likewise. > * config/aarch64/cygming.h (SYMBOL_FLAG_DLLIMPORT): Modify MinGW > environment to support DLL import/export. > (SYMBOL_FLAG_DLLEXPORT): Likewise. > (SYMBOL_REF_DLLIMPORT_P): Likewise. > (SYMBOL_FLAG_STUBVAR): Likewise. > (SYMBOL_REF_STUBVAR_P): Likewise. > (TARGET_VALID_DLLIMPORT_ATTRIBUTE_P): Likewise. > (TARGET_ASM_FILE_END): Likewise. > (SUB_TARGET_RECORD_STUB): Likewise. > (GOT_ALIAS_SET): Likewise. > (PE_COFF_EXTERN_DECL_SHOULD_BE_LEGITIMIZED): Likewise. > (HAVE_64BIT_POINTERS): Likewise. > --- > gcc/config.gcc | 4 +++- > gcc/config/aarch64/aarch64.cc | 37 +++++++++++++++++++++++++++++++++++ > gcc/config/aarch64/cygming.h | 26 ++++++++++++++++++++++-- > 3 files changed, 64 insertions(+), 3 deletions(-) > > diff --git a/gcc/config.gcc b/gcc/config.gcc > index d053b98efa8..331285b7b6d 100644 > --- a/gcc/config.gcc > +++ b/gcc/config.gcc > @@ -1276,10 +1276,12 @@ aarch64-*-mingw*) > tm_file="${tm_file} mingw/mingw32.h" > tm_file="${tm_file} mingw/mingw-stdint.h" > tm_file="${tm_file} mingw/winnt.h" > + tm_file="${tm_file} mingw/winnt-dll.h" > tmake_file="${tmake_file} aarch64/t-aarch64" > target_gtfiles="$target_gtfiles \$(srcdir)/config/mingw/winnt.cc" > + target_gtfiles="$target_gtfiles \$(srcdir)/config/mingw/winnt-dll.cc" > extra_options="${extra_options} mingw/cygming.opt mingw/mingw.opt" > - extra_objs="${extra_objs} winnt.o" > + extra_objs="${extra_objs} winnt.o winnt-dll.o" > c_target_objs="${c_target_objs} msformat-c.o" > d_target_objs="${d_target_objs} winnt-d.o" > tmake_file="${tmake_file} mingw/t-cygming" > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc > index 3418e57218f..5706b9aeb6b 100644 > --- a/gcc/config/aarch64/aarch64.cc > +++ b/gcc/config/aarch64/aarch64.cc > @@ -860,6 +860,10 @@ static const attribute_spec aarch64_gnu_attributes[] = > { "Advanced SIMD type", 1, 1, false, true, false, true, NULL, NULL }, > { "SVE type", 3, 3, false, true, false, true, NULL, NULL }, > { "SVE sizeless type", 0, 0, false, true, false, true, NULL, NULL }, > +#if TARGET_DLLIMPORT_DECL_ATTRIBUTES > + { "dllimport", 0, 0, false, false, false, false, handle_dll_attribute, NULL }, > + { "dllexport", 0, 0, false, false, false, false, handle_dll_attribute, NULL }, > +#endif > #ifdef SUBTARGET_ATTRIBUTE_TABLE > SUBTARGET_ATTRIBUTE_TABLE > #endif > @@ -2819,6 +2823,15 @@ tls_symbolic_operand_type (rtx addr) > return tls_kind; > } > > +rtx aarch64_legitimize_pe_coff_symbol (rtx addr, bool inreg) > +{ > +#if TARGET_PECOFF > + return legitimize_pe_coff_symbol (addr, inreg); > +#else > + return NULL_RTX; > +#endif > +} > + I wondered whether we should try to abstract this behind SUBTARGET_* stuff, e.g.: SUBTARGET_LEGITIMIZE_ADDRESS(ADDR) (the inreg==true case) SUBTARGET_LEGITIMIZE_CALLEE(ADDR) (the inreg==false case) But I don't think it falls out naturally with the way GCC's code is organised. I agree having direct references to PECOFF is probably the least worst option under the circumstances. Since there is no AArch64-specific handling, I think it'd be better to have: #if !TARGET_PECOFF rtx legitimize_pe_coff_symbol (rtx, bool) { return NULL_RTX; } #endif This avoids warning about unused arguments in the !TARGET_PECOFF case. > /* We'll allow lo_sum's in addresses in our legitimate addresses > so that combine would take care of combining addresses where > necessary, but for generation purposes, we'll generate the address > @@ -2865,6 +2878,17 @@ static void > aarch64_load_symref_appropriately (rtx dest, rtx imm, > enum aarch64_symbol_type type) > { > + /* If legitimize returns a value > + copy it directly to the destination and return. */ I don't think the comment really adds anything. > + > + rtx tmp = aarch64_legitimize_pe_coff_symbol (imm, true); > + Sorry for pushing personal preference, but I think it's slightly easier to read without this blank line (following the style used later in aarch64_legitimize_address). > + if (tmp) > + { > + emit_insn (gen_rtx_SET (dest, tmp)); > + return; Nit: these two lines should be indented by 6 spaces rather than 7. > + } > + > switch (type) > { > case SYMBOL_SMALL_ABSOLUTE: > @@ -11233,6 +11257,12 @@ aarch64_expand_call (rtx result, rtx mem, rtx cookie, bool sibcall) > > gcc_assert (MEM_P (mem)); > callee = XEXP (mem, 0); > + > + tmp = aarch64_legitimize_pe_coff_symbol (callee, false); > + Same comment about the blank line here. > + if (tmp) > + callee = tmp; > + > mode = GET_MODE (callee); > gcc_assert (mode == Pmode); > > @@ -12709,6 +12739,13 @@ aarch64_anchor_offset (HOST_WIDE_INT offset, HOST_WIDE_INT size, > static rtx > aarch64_legitimize_address (rtx x, rtx /* orig_x */, machine_mode mode) > { > + if (TARGET_DLLIMPORT_DECL_ATTRIBUTES) > + { > + rtx tmp = aarch64_legitimize_pe_coff_symbol (x, true); > + if (tmp) > + return tmp; > + } > + Is there a reason for guarding only this call with TARGET_DLLIMPORT_DECL_ATTRIBUTES? It seems a bit misleading, because nothing restricts legitimize_pe_coff_symbol to handling only the SYMBOL_REF_DLLIMPORT_P case. If the TARGET_DLLIMPORT_DECL_ATTRIBUTES condition can be dropped, the series is OK from my POV with that change and with the changes above. Please get sign-off from an x86 maintainer too though. Thanks, Richard > /* Try to split X+CONST into Y=X+(CONST & ~mask), Y+(CONST&mask), > where mask is selected by alignment and size of the offset. > We try to pick as large a range for the offset as possible to > diff --git a/gcc/config/aarch64/cygming.h b/gcc/config/aarch64/cygming.h > index 76623153080..e26488735db 100644 > --- a/gcc/config/aarch64/cygming.h > +++ b/gcc/config/aarch64/cygming.h > @@ -28,12 +28,18 @@ along with GCC; see the file COPYING3. If not see > > #define print_reg(rtx, code, file) (gcc_unreachable ()) > > -#define SYMBOL_FLAG_DLLIMPORT 0 > -#define SYMBOL_FLAG_DLLEXPORT 0 > +#define SYMBOL_FLAG_DLLIMPORT (SYMBOL_FLAG_MACH_DEP << 0) > +#define SYMBOL_REF_DLLIMPORT_P(X) \ > + ((SYMBOL_REF_FLAGS (X) & SYMBOL_FLAG_DLLIMPORT) != 0) > > +#define SYMBOL_FLAG_DLLEXPORT (SYMBOL_FLAG_MACH_DEP << 1) > #define SYMBOL_REF_DLLEXPORT_P(X) \ > ((SYMBOL_REF_FLAGS (X) & SYMBOL_FLAG_DLLEXPORT) != 0) > > +#define SYMBOL_FLAG_STUBVAR (SYMBOL_FLAG_MACH_DEP << 2) > +#define SYMBOL_REF_STUBVAR_P(X) \ > + ((SYMBOL_REF_FLAGS (X) & SYMBOL_FLAG_STUBVAR) != 0) > + > /* Disable SEH and declare the required SEH-related macros that are > still needed for compilation. */ > #undef TARGET_SEH > @@ -59,6 +65,12 @@ still needed for compilation. */ > #define TARGET_ASM_UNIQUE_SECTION mingw_pe_unique_section > #define TARGET_ENCODE_SECTION_INFO mingw_pe_encode_section_info > > +#define TARGET_VALID_DLLIMPORT_ATTRIBUTE_P mingw_pe_valid_dllimport_attribute_p > + > +/* Output function declarations at the end of the file. */ > +#undef TARGET_ASM_FILE_END > +#define TARGET_ASM_FILE_END mingw_pe_file_end > + > /* Declare the type properly for any external libcall. */ > #define ASM_OUTPUT_EXTERNAL_LIBCALL(FILE, FUN) \ > mingw_pe_declare_function_type (FILE, XSTR (FUN, 0), 1) > @@ -158,6 +170,9 @@ still needed for compilation. */ > { "selectany", 0, 0, true, false, false, false, \ > mingw_handle_selectany_attribute, NULL } > > +#undef SUB_TARGET_RECORD_STUB > +#define SUB_TARGET_RECORD_STUB mingw_pe_record_stub > + > #define SUPPORTS_ONE_ONLY 1 > > /* Define this to be nonzero if static stack checking is supported. */ > @@ -168,4 +183,11 @@ still needed for compilation. */ > #undef MAX_OFILE_ALIGNMENT > #define MAX_OFILE_ALIGNMENT (8192 * 8) > > +#undef GOT_ALIAS_SET > +#define GOT_ALIAS_SET mingw_GOT_alias_set () > + > +#define PE_COFF_EXTERN_DECL_SHOULD_BE_LEGITIMIZED 1 > + > +#define HAVE_64BIT_POINTERS 1 > + > #endif