Re: [PATCH v2 04/13] aarch64: Add aarch64-w64-mingw32 COFF

[Richard Sandiford <richard.sandiford@arm.com>]

Evgeny Karpov <Evgeny.Karpov@microsoft.com> writes:
> From: Zac Walker <zacwalker@microsoft.com>
> Date: Fri, 1 Mar 2024 01:55:47 +0100
> Subject: [PATCH v2 04/13] aarch64: Add aarch64-w64-mingw32 COFF
>
> Define ASM specific for COFF format on AArch64.
>
> gcc/ChangeLog:
>
> 	* config.gcc: Add COFF format support definitions.
> 	* config/aarch64/aarch64-coff.h: New file.
> ---
>  gcc/config.gcc                    |  1 +
>  gcc/config/aarch64/aarch64-coff.h | 91 +++++++++++++++++++++++++++++++
>  2 files changed, 92 insertions(+)
>  create mode 100644 gcc/config/aarch64/aarch64-coff.h
>
> diff --git a/gcc/config.gcc b/gcc/config.gcc
> index b762393b64c..cb6661f44ef 100644
> --- a/gcc/config.gcc
> +++ b/gcc/config.gcc
> @@ -1266,6 +1266,7 @@ aarch64*-*-linux*)
>  	;;
>  aarch64-*-mingw*)
>  	tm_file="${tm_file} aarch64/aarch64-abi-ms.h"
> +	tm_file="${tm_file} aarch64/aarch64-coff.h"
>  	tmake_file="${tmake_file} aarch64/t-aarch64"
>  	case ${enable_threads} in
>  	  "" | yes | win32)
> diff --git a/gcc/config/aarch64/aarch64-coff.h b/gcc/config/aarch64/aarch64-coff.h
> new file mode 100644
> index 00000000000..79c5a43b970
> --- /dev/null
> +++ b/gcc/config/aarch64/aarch64-coff.h
> @@ -0,0 +1,91 @@
> +/* Machine description for AArch64 architecture.
> +   Copyright (C) 2024 Free Software Foundation, Inc.
> +
> +   This file is part of GCC.
> +
> +   GCC is free software; you can redistribute it and/or modify it
> +   under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3, or (at your option)
> +   any later version.
> +
> +   GCC is distributed in the hope that it will be useful, but
> +   WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with GCC; see the file COPYING3.  If not see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef GCC_AARCH64_COFF_H
> +#define GCC_AARCH64_COFF_H
> +
> +#include "aarch64.h"

Is this needed?  It looks like aarch64-coff.h comes after aarch64.h
in the include list, so I'd have expected the #include to be a no-op.

If you want to emphasise that this file must be included after aarch64.h
then perhaps:

#if !defined(GCC_AARCH64_H)
#error This file must be included after aarch64.h
#endif

would work.  But it should also be ok just to drop the include without
replacing it with anything.

> +
> +#ifndef LOCAL_LABEL_PREFIX
> +# define LOCAL_LABEL_PREFIX 	""
> +#endif
> +
> +/* Using long long breaks -ansi and -std=c90, so these will need to be
> +   made conditional for an LLP64 ABI.  */
> +#undef SIZE_TYPE
> +#define SIZE_TYPE	"long long unsigned int"
> +
> +#undef PTRDIFF_TYPE
> +#define PTRDIFF_TYPE	"long long int"
> +
> +#undef LONG_TYPE_SIZE
> +#define LONG_TYPE_SIZE 32
> +
> +#ifndef ASM_GENERATE_INTERNAL_LABEL
> +# define ASM_GENERATE_INTERNAL_LABEL(STRING, PREFIX, NUM)  \
> +  sprintf (STRING, "*%s%s%u", LOCAL_LABEL_PREFIX, PREFIX, (unsigned int)(NUM))
> +#endif
> +
> +#define ASM_OUTPUT_ALIGN(STREAM, POWER)		\
> +  fprintf (STREAM, "\t.align\t%d\n", (int)POWER)
> +
> +/* Output a common block.  */
> +#ifndef ASM_OUTPUT_COMMON
> +# define ASM_OUTPUT_COMMON(STREAM, NAME, SIZE, ROUNDED)	\
> +    {							\
> +      fprintf (STREAM, "\t.comm\t");			\
> +      assemble_name (STREAM, NAME);			\
> +      asm_fprintf (STREAM, ", %d, %d\n", 		\
> +      (int)(ROUNDED), (int)(SIZE));	\
> +    }
> +#endif
> +
> +/* Output a local common block.  /bin/as can't do this, so hack a
> +   `.space' into the bss segment.  Note that this is *bad* practice,
> +   which is guaranteed NOT to work since it doesn't define STATIC
> +   COMMON space but merely STATIC BSS space.  */
> +#ifndef ASM_OUTPUT_ALIGNED_LOCAL
> +# define ASM_OUTPUT_ALIGNED_LOCAL(STREAM, NAME, SIZE, ALIGN)		\
> +    {									\
> +      switch_to_section (bss_section);					\
> +      ASM_OUTPUT_ALIGN (STREAM, floor_log2 (ALIGN / BITS_PER_UNIT));	\
> +      ASM_OUTPUT_LABEL (STREAM, NAME);					\
> +      fprintf (STREAM, "\t.space\t%d\n", (int)(SIZE));			\
> +    }
> +#endif
> +
> +#define ASM_OUTPUT_SKIP(STREAM, NBYTES) 	\
> +  fprintf (STREAM, "\t.space\t%d  // skip\n", (int) (NBYTES))
> +
> +#define ASM_OUTPUT_TYPE_DIRECTIVE(STREAM, NAME, TYPE)
> +#define ASM_DECLARE_FUNCTION_SIZE(FILE, FNAME, DECL)

Just curious: are these empty definitions the final intended
definitions, or are they just temporary?  Might be worth a comment
either way.

Thanks,
Richard

> +
> +#define TEXT_SECTION_ASM_OP	"\t.text"
> +#define DATA_SECTION_ASM_OP	"\t.data"
> +#define BSS_SECTION_ASM_OP	"\t.bss"
> +
> +#define CTORS_SECTION_ASM_OP	"\t.section\t.ctors, \"aw\""
> +#define DTORS_SECTION_ASM_OP	"\t.section\t.dtors, \"aw\""
> +
> +#define GLOBAL_ASM_OP "\t.global\t"
> +
> +#undef SUPPORTS_INIT_PRIORITY
> +#define SUPPORTS_INIT_PRIORITY 0
> +
> +#endif