* [patch][gcn] mkoffload: Fix linking with "-g"; fix file deletion; improve diagnostic [PR111966]
@ 2024-01-19 20:07 Tobias Burnus
2024-01-22 7:42 ` Richard Biener
0 siblings, 1 reply; 2+ messages in thread
From: Tobias Burnus @ 2024-01-19 20:07 UTC (permalink / raw)
To: gcc-patches, Andrew Stubbs
[-- Attachment #1: Type: text/plain, Size: 855 bytes --]
This patch fixes PR111966, i.e. when compiling offloaded code with "-g"
but without "-march=", mkoffload created a file with e_flags set to
gfx803/fiji as architecture - while all other files used gfx900, which
the linker did not like.
Reason: When the default was changed, this flag was missed. When passing
-march=... instead of relying on the default, it works.
Additionally, it fixed a bug with dangling pointers and multiple
deletion attempts for the same file, leading normally only to the
accumulation of /tmp/cc*.mkoffload.dbg.o files.
And, finally, when building with a recent GCC; it warned about missing
%<...%> or %qs quotes. I added a couple to reduce the number of warnings.
OK for mainline? — I think the /tmp/cc*.mkoffload.dbg.o part of the
patch could also be backported to GCC 13 (and 12) if deemed to be useful.
Tobias
[-- Attachment #2: mkoffload-gcn-fix.diff --]
[-- Type: text/x-patch, Size: 6500 bytes --]
[gcn] mkoffload: Fix linking with "-g"; fix file deletion; improve diagnostic [PR111966]
With debugging enabled, '*.mkoffload.dbg.o' files are generated. The e_flags
header of all *.o files must be the same - otherwise, the linker complains.
Since r14-4734-g56ed1055b2f40ac162ae8d382280ac07a33f789f the -march= default
is now gfx900. If compiling without any -march= flag, the default value is
used by the compiler but not passed to mkoffload. Hence, mkoffload.cc's uses
its own default for march - unfortunately, it still had gfx803/fiji as default,
leading to the linker error: 'incompatible mach'. Solution: Update the
default to gfx900.
While debugging it, I saw that /tmp/cc*.mkoffload.dbg.o kept accumulating;
there were a couple of issues with the handling:
* dbgobj was always added to files_to_cleanup
* If copy_early_debug_info returned true, dbgobj was added again
-> pointless and in theory a race if the same file was added in the
faction of a second.
* If copy_early_debug_info returned false,
- In exactly one case, it already deleted the file it self
(same potential race as above)
- The pointer dbgobj was freed - such that files_to_cleanup contained
a dangling pointer - probably the reason that stale files remained.
Solution: Only if copy_early_debug_info returns true, dbgobj is added to
files_to_cleanup. If it returns false, the file is unlinked before freeing
the pointer.
When compiling, GCC warned about several fatal_error messages as having
no %<...%> or %qs quotes. This patch now silences several of those warnings
by using those quotes.
gcc/ChangeLog:
PR other/111966
* config/gcn/mkoffload.cc (elf_arch): Change default to gfx900
to match the compiler default.
(simple_object_copy_lto_debug_sections): Never unlink the outfile
on error as the caller does so.
(maybe_unlink, compile_native): Use %<...%> and %qs in fatal_error.
(main): Likewise. Fix 'mkoffload.dbg.o' cleanup.
Signed-off-by: Tobias Burnus <tburnus@baylibre.com>
gcc/config/gcn/mkoffload.cc | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/gcc/config/gcn/mkoffload.cc b/gcc/config/gcn/mkoffload.cc
index d4cd509089e..0d0e7bac9b2 100644
--- a/gcc/config/gcn/mkoffload.cc
+++ b/gcc/config/gcn/mkoffload.cc
@@ -124,7 +124,7 @@ static const char *gcn_dumpbase;
static struct obstack files_to_cleanup;
enum offload_abi offload_abi = OFFLOAD_ABI_UNSET;
-uint32_t elf_arch = EF_AMDGPU_MACH_AMDGCN_GFX803; // Default GPU architecture.
+uint32_t elf_arch = EF_AMDGPU_MACH_AMDGCN_GFX900; // Default GPU architecture.
uint32_t elf_flags = EF_AMDGPU_FEATURE_SRAMECC_ANY_V4;
static int gcn_stack_size = 0; /* Zero means use default. */
@@ -154,7 +154,7 @@ maybe_unlink (const char *file)
if (!save_temps)
{
if (unlink_if_ordinary (file) && errno != ENOENT)
- fatal_error (input_location, "deleting file %s: %m", file);
+ fatal_error (input_location, "deleting file %qs: %m", file);
}
else if (verbose)
fprintf (stderr, "[Leaving %s]\n", file);
@@ -320,10 +320,7 @@ copy_early_debug_info (const char *infile, const char *outfile)
errmsg = simple_object_copy_lto_debug_sections (inobj, outfile, &err, true);
if (errmsg)
- {
- unlink_if_ordinary (outfile);
- return false;
- }
+ return false;
simple_object_release_read (inobj);
close (infd);
@@ -804,7 +801,7 @@ compile_native (const char *infile, const char *outfile, const char *compiler,
const char *collect_gcc_options = getenv ("COLLECT_GCC_OPTIONS");
if (!collect_gcc_options)
fatal_error (input_location,
- "environment variable COLLECT_GCC_OPTIONS must be set");
+ "environment variable %<COLLECT_GCC_OPTIONS%> must be set");
struct obstack argv_obstack;
obstack_init (&argv_obstack);
@@ -859,11 +856,11 @@ main (int argc, char **argv)
obstack_init (&files_to_cleanup);
if (atexit (mkoffload_cleanup) != 0)
- fatal_error (input_location, "atexit failed");
+ fatal_error (input_location, "%<atexit%> failed");
char *collect_gcc = getenv ("COLLECT_GCC");
if (collect_gcc == NULL)
- fatal_error (input_location, "COLLECT_GCC must be set.");
+ fatal_error (input_location, "%<COLLECT_GCC%> must be set");
const char *gcc_path = dirname (ASTRDUP (collect_gcc));
const char *gcc_exec = basename (ASTRDUP (collect_gcc));
@@ -909,7 +906,7 @@ main (int argc, char **argv)
if (!found)
fatal_error (input_location,
- "offload compiler %s not found", GCC_INSTALL_NAME);
+ "offload compiler %qs not found", GCC_INSTALL_NAME);
/* We may be called with all the arguments stored in some file and
passed with @file. Expand them into argv before processing. */
@@ -931,7 +928,7 @@ main (int argc, char **argv)
offload_abi = OFFLOAD_ABI_ILP32;
else
fatal_error (input_location,
- "unrecognizable argument of option " STR);
+ "unrecognizable argument of option %<" STR "%>");
}
#undef STR
else if (strcmp (argv[i], "-fopenmp") == 0)
@@ -994,7 +991,8 @@ main (int argc, char **argv)
}
if (!(fopenacc ^ fopenmp))
- fatal_error (input_location, "either -fopenacc or -fopenmp must be set");
+ fatal_error (input_location,
+ "either %<-fopenacc%> or %<-fopenmp%> must be set");
const char *abi;
switch (offload_abi)
@@ -1066,7 +1064,7 @@ main (int argc, char **argv)
cfile = fopen (gcn_cfile_name, "w");
if (!cfile)
- fatal_error (input_location, "cannot open '%s'", gcn_cfile_name);
+ fatal_error (input_location, "cannot open %qs", gcn_cfile_name);
/* Currently, we only support offloading in 64-bit configurations. */
if (offload_abi == OFFLOAD_ABI_LP64)
@@ -1130,7 +1128,6 @@ main (int argc, char **argv)
}
else
dbgobj = make_temp_file (".mkoffload.dbg.o");
- obstack_ptr_grow (&files_to_cleanup, dbgobj);
/* If the copy fails then just ignore it. */
if (copy_early_debug_info (argv[ix], dbgobj))
@@ -1139,7 +1136,10 @@ main (int argc, char **argv)
obstack_ptr_grow (&files_to_cleanup, dbgobj);
}
else
- free (dbgobj);
+ {
+ maybe_unlink (dbgobj);
+ free (dbgobj);
+ }
}
}
}
@@ -1214,7 +1214,7 @@ main (int argc, char **argv)
out = fopen (gcn_s2_name, "w");
if (!out)
- fatal_error (input_location, "cannot open '%s'", gcn_s2_name);
+ fatal_error (input_location, "cannot open %qs", gcn_s2_name);
process_asm (in, out, cfile);
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [patch][gcn] mkoffload: Fix linking with "-g"; fix file deletion; improve diagnostic [PR111966]
2024-01-19 20:07 [patch][gcn] mkoffload: Fix linking with "-g"; fix file deletion; improve diagnostic [PR111966] Tobias Burnus
@ 2024-01-22 7:42 ` Richard Biener
0 siblings, 0 replies; 2+ messages in thread
From: Richard Biener @ 2024-01-22 7:42 UTC (permalink / raw)
To: Tobias Burnus; +Cc: gcc-patches, Andrew Stubbs
On Fri, Jan 19, 2024 at 9:08 PM Tobias Burnus <tburnus@baylibre.com> wrote:
>
> This patch fixes PR111966, i.e. when compiling offloaded code with "-g"
> but without "-march=", mkoffload created a file with e_flags set to
> gfx803/fiji as architecture - while all other files used gfx900, which
> the linker did not like.
>
> Reason: When the default was changed, this flag was missed. When passing
> -march=... instead of relying on the default, it works.
>
> Additionally, it fixed a bug with dangling pointers and multiple
> deletion attempts for the same file, leading normally only to the
> accumulation of /tmp/cc*.mkoffload.dbg.o files.
>
> And, finally, when building with a recent GCC; it warned about missing
> %<...%> or %qs quotes. I added a couple to reduce the number of warnings.
>
> OK for mainline? — I think the /tmp/cc*.mkoffload.dbg.o part of the
> patch could also be backported to GCC 13 (and 12) if deemed to be useful.
OK for trunk
> Tobias
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2024-01-22 7:44 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-19 20:07 [patch][gcn] mkoffload: Fix linking with "-g"; fix file deletion; improve diagnostic [PR111966] Tobias Burnus
2024-01-22 7:42 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).