<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">
Hi Rafael.
<div class=""><br class="">
</div>
<div class="">Thanks for your comments. I’ve answered inline below.</div>
<div class="">B.T.W. It seems emails are not showing up in phabricator.</div>
<div class=""><br class="">
<div>
<blockquote type="cite" class="">
<div class="">On 23 Feb 2017, at 13:20, Rafael Avila de Espindola <<a href="mailto:rafael.espindola@gmail.com" class="">rafael.espindola@gmail.com</a>> wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<div class="">Christof Douma via Phabricator <<a href="mailto:reviews@reviews.llvm.org" class="">reviews@reviews.llvm.org</a>> writes:<br class="">
<blockquote type="cite" class="">- MCSection *ELFSection = getContext().getELFSection(<br class="">
+ MCSectionELF *ELFSection = getContext().getELFSection(<br class="">
SectionName, Type, Flags, Size, GroupName, UniqueID, Associated);<br class="">
+ // Sections are not identified by flags. Check if there is a mismatch in the<br class="">
+ // flags requested and the flags in the section returned. If the mismatch is<br class="">
+ // in OS or CPU specific flags, just add them. For any other mismatch warn<br class="">
+ // the user that we are ignoring the new flags.<br class="">
+ const unsigned overrideMask = (ELF::SHF_MASKOS | ELF::SHF_MASKPROC);<br class="">
+ unsigned curFlags = ELFSection->getFlags();<br class="">
+ curFlags |= Flags & overrideMask;<br class="">
+ Flags |= curFlags & overrideMask;<br class="">
+ ELFSection->setFlags(curFlags);<br class="">
+ if (curFlags != Flags)<br class="">
+ Warning(loc, "ignoring override of non-target sepecific section flags");<br class="">
+<br class="">
getStreamer().SwitchSection(ELFSection, Subsection);<br class="">
</blockquote>
<br class="">
Are you should this is an intentional behaviour? It is hard to see why<br class="">
it is a good thing for MASKPROC and MASKOS to be special.<br class="">
</div>
</div>
</blockquote>
<div><br class="">
</div>
<div>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. </div>
<div><br class="">
</div>
<div>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.</div>
</div>
<div><br class="">
[1] <a href="https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gas/config/obj-elf.c;h=9af349c2c167e7d723bd4eeabe421ff7eb8d2f34;hb=9af349c2c167e7d723bd4eeabe421ff7eb8d2f34#l741" class="">https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gas/config/obj-elf.c;h=9af349c2c167e7d723bd4eeabe421ff7eb8d2f34;hb=9af349c2c167e7d723bd4eeabe421ff7eb8d2f34#l741</a><br class="">
<br class="">
<blockquote type="cite" class="">
<div class="">
<div class=""><br class="">
<blockquote type="cite" class=""> if (getContext().getGenDwarfForAssembly()) {<br class="">
<br class="">
<br class="">
Index: test/MC/ELF/section-override-flags.s<br class="">
===================================================================<br class="">
--- /dev/null<br class="">
+++ test/MC/ELF/section-override-flags.s<br class="">
@@ -0,0 +1,36 @@<br class="">
+// RUN: llvm-mc -filetype=obj -triple arm-pc-linux-gnu %s -o %t 2>&1 | FileCheck -check-prefix=DIAG %s<br class="">
+// RUN: llvm-readobj -s %t | FileCheck %s<br class="">
+<br class="">
+// REQUIRES: arm-registered-target<br class="">
</blockquote>
<br class="">
Just move the test to an ARM subdirectory.<br class="">
</div>
</div>
</blockquote>
<div><br class="">
</div>
<div>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 ;-)<br class="">
</div>
<br class="">
<blockquote type="cite" class="">
<div class="">
<div class=""><br class="">
<blockquote type="cite" class="">+// Test we handle overriding OS and PROC specific flags on the builtin .text section<br class="">
+.section .text,"0x200006",%progbits<br class="">
+.section .text,"0x40000006",%progbits<br class="">
</blockquote>
<br class="">
<br class="">
The distinction of builtin versus regular is also odd. The ELF spec has<br class="">
no such terms.<br class="">
</div>
</div>
</blockquote>
<div><br class="">
</div>
<div>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.</div>
<br class="">
<blockquote type="cite" class="">
<div class="">
<div class=""><br class="">
<br class="">
<blockquote type="cite" class="">+// CHECK: Section {<br class="">
+// CHECK: Name: .text<br class="">
+// CHECK-NEXT: Type: SHT_PROGBITS (0x1)<br class="">
+// CHECK-NEXT: Flags [ (0x40200006)<br class="">
+// CHECK-NEXT: SHF_ALLOC (0x2)<br class="">
+// CHECK-NEXT: SHF_EXECINSTR (0x4)<br class="">
+// CHECK-NEXT: ]<br class="">
+<br class="">
+//Attempt to override flags that are not target specific should be ignored<br class="">
+.section foo,"ax",%progbits<br class="">
+.section foo,"0x7",%progbits<br class="">
+.section foo,"0x10000007",%progbits<br class="">
+<br class="">
+// DIAG: warning: ignoring override of non-target sepecific section flags<br class="">
+// DIAG: warning: ignoring override of non-target sepecific section flags<br class="">
</blockquote>
<br class="">
Test the location.<br class="">
</div>
</div>
</blockquote>
<div><br class="">
</div>
<div>OK.</div>
<div><br class="">
</div>
<div>I’ll update the patch in phabricator with your comments shortly.<br class="">
</div>
<div><br class="">
</div>
<div>Thanks,</div>
<div>Christof</div>
<br class="">
<blockquote type="cite" class="">
<div class="">
<div class=""><br class="">
Cheers,<br class="">
Rafael<br class="">
</div>
</div>
</blockquote>
</div>
<br class="">
</div>
</body>
</html>