[PATCH] D30291: Handle section header flags redefinitions similar to GAS

[Christof Douma via llvm-commits]

Hi Rafael.

Thanks for your comments. I’ve answered inline below.
B.T.W. It seems emails are not showing up in phabricator.

On 23 Feb 2017, at 13:20, Rafael Avila de Espindola <rafael.espindola at gmail.com<mailto:rafael.espindola at gmail.com>> wrote:

Christof Douma via Phabricator <reviews at reviews.llvm.org<mailto:reviews at reviews.llvm.org>> writes:
-  MCSection *ELFSection = getContext().getELFSection(
+  MCSectionELF *ELFSection = getContext().getELFSection(
      SectionName, Type, Flags, Size, GroupName, UniqueID, Associated);
+  // Sections are not identified by flags. Check if there is a mismatch in the
+  // flags requested and the flags in the section returned. If the mismatch is
+  // in OS or CPU specific flags, just add them. For any other mismatch warn
+  // the user that we are ignoring the new flags.
+  const unsigned overrideMask = (ELF::SHF_MASKOS | ELF::SHF_MASKPROC);
+  unsigned curFlags = ELFSection->getFlags();
+  curFlags |= Flags & overrideMask;
+  Flags |= curFlags & overrideMask;
+  ELFSection->setFlags(curFlags);
+  if (curFlags != Flags)
+    Warning(loc, "ignoring override of non-target sepecific section flags");
+
  getStreamer().SwitchSection(ELFSection, Subsection);

Are you should this is an intentional behaviour? It is hard to see why
it is a good thing for MASKPROC and MASKOS to be special.

The code that controls this is at [1]. It diagnoses flag overrides for some libbfd specific macro’s, but it accepts the rest. From the comment in the else clause there and some chats with engineers that worked on GAS I believe the argument in GAS is that MASKPROC and MASKOS are special because they are target-specific flags in the ELF standard, while the other are all defined there.

I am not 100% certain about which flags to accept and which to diagnose as there is no definite spec that states what the assembly directive is supposed to do. But this idea to allow target-specific flags to be overridden sounded reasonable enough to me. At the moment we silently ignore an attempt to override section header flags, which I think is the worst kind of treatment.

[1] https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gas/config/obj-elf.c;h=9af349c2c167e7d723bd4eeabe421ff7eb8d2f34;hb=9af349c2c167e7d723bd4eeabe421ff7eb8d2f34#l741


  if (getContext().getGenDwarfForAssembly()) {


Index: test/MC/ELF/section-override-flags.s
===================================================================
--- /dev/null
+++ test/MC/ELF/section-override-flags.s
@@ -0,0 +1,36 @@
+// RUN: llvm-mc -filetype=obj -triple arm-pc-linux-gnu %s -o %t 2>&1 | FileCheck -check-prefix=DIAG %s
+// RUN: llvm-readobj -s %t | FileCheck %s
+
+// REQUIRES: arm-registered-target

Just move the test to an ARM subdirectory.

OK. I thought that would make people think it is ARM specific. This test is not ARM specific, I just needed any target that generates ELF. Somehow ARM came to my mind first ;-)


+// Test we handle overriding OS and PROC specific flags on the builtin .text section
+.section .text,"0x200006",%progbits
+.section .text,"0x40000006",%progbits


The distinction of builtin versus regular is also odd. The ELF spec has
no such terms.

It is not the ELF spec that I refer to. It is LLVM that has some pre-constructred sections (see: MCObjectFileInfo::initELFMCObjectFileInfo()). For example: the “.text” section exists before even using any directive . I wanted to cover that in this test too.



+// CHECK:      Section {
+// CHECK:        Name: .text
+// CHECK-NEXT:   Type: SHT_PROGBITS (0x1)
+// CHECK-NEXT:   Flags [ (0x40200006)
+// CHECK-NEXT:     SHF_ALLOC (0x2)
+// CHECK-NEXT:     SHF_EXECINSTR (0x4)
+// CHECK-NEXT:   ]
+
+//Attempt to override flags that are not target specific should be ignored
+.section foo,"ax",%progbits
+.section foo,"0x7",%progbits
+.section foo,"0x10000007",%progbits
+
+// DIAG: warning: ignoring override of non-target sepecific section flags
+// DIAG: warning: ignoring override of non-target sepecific section flags

Test the location.

OK.

I’ll update the patch in phabricator with your comments shortly.

Thanks,
Christof


Cheers,
Rafael

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170223/a27b7901/attachment.html>