Bug 97827 - bootstrap error building the amdgcn-amdhsa offload compiler with LLVM 11
Summary: bootstrap error building the amdgcn-amdhsa offload compiler with LLVM 11
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 11.0
: P3 normal
Target Milestone: 11.0
Assignee: Not yet assigned to anyone
URL:
Keywords: build
Depends on:
Blocks:
 
Reported: 2020-11-14 11:02 UTC by Matthias Klose
Modified: 2021-01-28 15:21 UTC (History)
6 users (show)

See Also:
Host:
Target: gcn
Build:
Known to work:
Known to fail:
Last reconfirmed: 2020-12-27 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Klose 2020-11-14 11:02:53 UTC
builds fine with trunk 20201112, fails with trunk 20201113. newlib 3.3.0 is used.

[...]
/tmp/ccpzv56q.s:1129:2: error: changed section flags for .rodata.cst8, expected: 0x12
        .section        .rodata.cst8
        ^
/tmp/ccpzv56q.s:1129:2: error: changed section entsize for .rodata.cst8, expected: 8
        .section        .rodata.cst8
        ^
make[10]: *** [Makefile:980: lib_a-wcstod.o] Error 1
make[10]: *** Waiting for unfinished jobs....
/tmp/ccDmKs3n.s:4450:2: error: changed section flags for .rodata.cst8, expected: 0x12
        .section        .rodata.cst8
        ^
/tmp/ccDmKs3n.s:4450:2: error: changed section entsize for .rodata.cst8, expected: 8
        .section        .rodata.cst8
        ^
/tmp/ccDmKs3n.s:4643:2: error: changed section flags for .rodata.cst8, expected: 0x12
        .section        .rodata.cst8
        ^
/tmp/ccDmKs3n.s:4643:2: error: changed section entsize for .rodata.cst8, expected: 8
        .section        .rodata.cst8
        ^
make[10]: *** [Makefile:944: lib_a-strtod.o] Error 1
make[10]: Leaving directory '/packages/gcc/11/gcc-11-11-20201113/build-gcn/amdgcn-amdhsa/gfx900/newlib/libc/stdlib'
Comment 1 Matthias Klose 2020-11-16 08:06:52 UTC
my bad, this is not a regression.  Seen when building the offload compiler with LLVM 11 instead of LLVM 10 or 9
Comment 2 Tobias Burnus 2020-11-17 11:44:46 UTC
Changed in LLVM in https://reviews.llvm.org/D73999
cf. https://lists.llvm.org/pipermail/llvm-dev/2020-February/139390.html
and related Binutils discussion:
https://sourceware.org/legacy-ml/binutils/2020-02/msg00093.html

LLVM commit 75af9da755721123e62b45cd0bc0c5e688a9722a:
"   GNU as started to emit warnings for changed sh_type or sh_flags in 2000.
    GNU as>=2.35 will emit errors for most sh_type/sh_flags change, and error for entsize change."

Cf. also http://web.mit.edu/rhel-doc/3/rhel-as-en-3/section.html



A grep shows:
        .section        .rodata.cst8
        .section        .rodata.cst8
        .section        .rodata.cst8,"aM",@progbits,8

Codewise, we have:
  mergeable_constant_section (mode=E_DFmode, align=64, flags=0)
which calls:
  884           flags |= (align / 8) | SECTION_MERGE;
  885           return get_section (name, flags, NULL);
with flags == 32776.

In the latter:
      flags |= SECTION_NAMED;
  → 2129928
and as "slot = section_htab->find_slot_with_hash" → *slot == NULL:
      sect = ggc_alloc<section> ();
      sect->named.common.flags = flags;
      sect->named.name = ggc_strdup (name);
      sect->named.decl = decl;

However, once the same function is called, we have:
  sect->common.flags == 3178504
while still/again:
  flags == 2129928

Next comes the call to:
  switch_to_section
which ends with
  new_section->common.flags |= SECTION_DECLARED;
which is
  3178504


If we look at default_elf_asm_named_section, we find:

  /* If we have already declared this section, we can use an
     abbreviated form to switch back to it -- unless this section is
     part of a COMDAT groups, in which case GAS requires the full
     declaration every time.  */
  if (!(HAVE_COMDAT_GROUP && (flags & SECTION_LINKONCE))
      && (flags & SECTION_DECLARED))
    { 
      fprintf (asm_out_file, "\t.section\t%s\n", name);
      return;
    }
Comment 3 Tobias Burnus 2020-11-17 12:45:19 UTC
Now filled  https://bugs.llvm.org/show_bug.cgi?id=48201  for this

LLVM and GNU as ("gas") tried to align, cf. discussion
  https://sourceware.org/legacy-ml/binutils/2020-02/msg00091.html
and the GAS patch to turn a warning to an error:
  https://sourceware.org/legacy-ml/binutils/2020-02/msg00129.html


However, while llvm/lib/MC/MCParser/ELFAsmParser.cpp always issues an error:

+  if (Section->getType() != Type)
+    Error(loc, "changed section type for " + SectionName + ", expected: 0x" +
+                   utohexstr(Section->getType()));
etc.

The GNU assembler only does so if flags have been specified,
gas/config/obj-elf.c:

      if (attr != 0)
        { 
          /* If section attributes are specified the second time we see a
             particular section, then check that they are the same as we
             saw the first time.  */
          if (((old_sec->flags ^ flags)
...
Comment 4 Tobias Burnus 2020-11-17 16:31:42 UTC
Draft patch (untested):

diff --git a/gcc/varasm.c b/gcc/varasm.c
index ada99940f65..51a507393a8 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -6738,9 +6738,11 @@ default_elf_asm_named_section (const char *name, unsigned int flags,
   /* If we have already declared this section, we can use an
      abbreviated form to switch back to it -- unless this section is
      part of a COMDAT groups, in which case GAS requires the full
-     declaration every time.  */
+     declaration every time.  LLVM's MC linker requires that the
+     flags are identical, thus avoid the abbreviated form with MERGE.  */
   if (!(HAVE_COMDAT_GROUP && (flags & SECTION_LINKONCE))
-      && (flags & SECTION_DECLARED))
+      && (flags & SECTION_DECLARED)
+      && !(flags & SECTION_MERGE))
     {
       fprintf (asm_out_file, "\t.section\t%s\n", name);
       return;
Comment 5 Tobias Burnus 2020-11-18 12:32:42 UTC
Patch: https://gcc.gnu.org/pipermail/gcc-patches/2020-November/559487.html

First review regards this as LLVM bug.
Comment 6 Jakub Jelinek 2020-11-18 12:46:51 UTC
Might not be a LLVM bug if llvm-as doesn't pretend to be assembler compatible with other assemblers.  But in that case we should treat it as a target with specific assembler requirements (e.g. like Darwin).
Comment 7 Tobias Burnus 2020-11-24 20:01:36 UTC
Submitted LLVM patch at https://reviews.llvm.org/D92052
If it gets accepted for LLVM + backported to 11, we are done.

Otherwise, we have to proceed as suggested in the email thread.
Comment 8 Tobias Burnus 2020-12-11 17:02:22 UTC
FIXED in LLVM 'main' in commit 1deff4009e0ae661b03682901bf6932297ce7ea1
→ https://reviews.llvm.org/D92052 + https://bugs.llvm.org/show_bug.cgi?id=48201

Close as WONTFIX on the GCC side.

I try to get it backported to LLVM 11.0.1 or, failing that, LLVM 11.0.2.

Thanks Matthias for pointing out the issue.
Comment 9 Fangrui Song 2020-12-14 18:31:16 UTC
I want to know whether this is really a wontfix on GCC's side.

Richard Sandiford on https://gcc.gnu.org/pipermail/gcc-patches/2020-November/559572.html

"I'm not saying we should bend over backwards to support difficult
quirks.  But here we're talking about a choice between (a) doing
something that works “everywhere” unconditionally (and keeping things
simple) vs. (b) having both code that takes a shortcut and code that
doesn't take a shortcut and trying to predict which one we should do."

This makes a lot of sense to me. For the LLVM "fix", we had not known this PR before https://reviews.llvm.org/D92052#2452577
To me personally, I might have a different opinion if I knew this is not an entire dead end on gcc -S output.
Comment 10 Fangrui Song 2020-12-14 18:38:06 UTC
Note: the section key is not just (name, group name "G"). It is a quadruple: (name, group name "G", linked-to "o", unique ID)

Keeping just name works for the simplest case.

If GCC decides to support PR95095  -fno-unique-section-names, unique ID can be common. https://sourceware.org/bugzilla/show_bug.cgi?id=25490#c3 added the support for `.section ....,unique` to GNU as.
Comment 11 Tobias Burnus 2020-12-15 10:26:33 UTC
For completeness, the LLVM 'main' patch was backported/cherry-picked for
LLVM 11.0.1 with commit 700baa009dc685a0adc5f94d258be4ae24742471

Regarding the .section discussion, see also last few comments in https://reviews.llvm.org/D92052

Thanks again for the bug report report, Matthias – and to those providing background information and helping to reach a good solution :-)
Comment 12 Matthias Klose 2020-12-22 16:17:01 UTC
Fyi, this is still seen with LLVM 11.0.1 rc2
Comment 13 Tobias Burnus 2020-12-27 20:26:02 UTC
(In reply to Matthias Klose from comment #12)
> Fyi, this is still seen with LLVM 11.0.1 rc2

But the fix (commit 700baa009dc685a0adc5f94d258be4ae24742471) is included in llvmorg-11.0.1-rc2.

If the issue is still present, something else goes wrong. What's the error message? Is it really identical to the one of comment 0?
Comment 14 Matthias Klose 2020-12-28 10:12:34 UTC
[...]
/tmp/ccQFzQVM.s:1082:2: error: changed section flags for .rodata.cst8, expected:
0x12
        .section        .rodata.cst8
        ^
/tmp/ccQFzQVM.s:1082:2: error: changed section entsize for .rodata.cst8, expected: 8
        .section        .rodata.cst8
        ^
make[10]: *** [Makefile:980: lib_a-wcstod.o] Error 1
Comment 15 Tobias Burnus 2021-01-05 10:31:15 UTC
I unfortunately missed in my the LLVM patch that '.rodata' implies flags and messed up the check. Should by fixed by: https://reviews.llvm.org/D94072
Comment 16 Tobias Burnus 2021-01-28 15:21:54 UTC
(In reply to Tobias Burnus from comment #15)
> I unfortunately missed in my the LLVM patch that '.rodata' implies flags and
> messed up the check. Should by fixed by: https://reviews.llvm.org/D94072

Now merged to LLVM 12/trunk (see ↑ or next link); for LLVM 11 backporting, see:
https://bugs.llvm.org/show_bug.cgi?id=48922

For related Debian bug, see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=975692

Close as FIXED – hopefully, now for real.