<div dir="ltr">I would be fine with that personally. I added this to fix an issue we had with SHN_ABS. I CC'd Roland to get his opinion as well.<br><br><div class="gmail_quote"><div dir="ltr">On Mon, Sep 11, 2017 at 4:09 PM Sean Silva <<a href="mailto:chisophugis@gmail.com">chisophugis@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto">For now we can restrict it to SHN_ABS and SHN_COMMON, no?</div><div class="gmail_extra"><br><div class="gmail_quote">On Sep 11, 2017 3:13 PM, "Jake Ehrlich via llvm-commits" <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br type="attribution"><blockquote class="m_4152240823519784738quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">This is discussed a bit in review here: <a href="https://reviews.llvm.org/D37393" target="_blank">https://reviews.llvm.org/D37393</a><br><br>Basically Roland was uncomfortable with assuming we can handle everything above SHN_LORESERVE uniformly. To quote Roland<br>"""<br><span style="color:rgb(70,76,92);font-family:"Segoe UI","Segoe UI Emoji","Segoe UI Symbol",Lato,"Helvetica Neue",Helvetica,Arial,sans-serif">It's probably OK to just pass all other >= SHN_LORESERVE values through blindly, but I'm not sure it's the best idea.</span><br style="margin-top:0px;color:rgb(70,76,92);font-family:"Segoe UI","Segoe UI Emoji","Segoe UI Symbol",Lato,"Helvetica Neue",Helvetica,Arial,sans-serif"><span style="color:rgb(70,76,92);font-family:"Segoe UI","Segoe UI Emoji","Segoe UI Symbol",Lato,"Helvetica Neue",Helvetica,Arial,sans-serif">Their meaning and appropriate handling depends on each specific value defined by a particular machine or OS.</span><br style="color:rgb(70,76,92);font-family:"Segoe UI","Segoe UI Emoji","Segoe UI Symbol",Lato,"Helvetica Neue",Helvetica,Arial,sans-serif"><span style="color:rgb(70,76,92);font-family:"Segoe UI","Segoe UI Emoji","Segoe UI Symbol",Lato,"Helvetica Neue",Helvetica,Arial,sans-serif">For all the ones I'm familiar with, just passing it through is all you need to do. But I'm not entirely sanguine about assuming that uniformly.</span><br style="color:rgb(70,76,92);font-family:"Segoe UI","Segoe UI Emoji","Segoe UI Symbol",Lato,"Helvetica Neue",Helvetica,Arial,sans-serif"><span style="color:rgb(70,76,92);font-family:"Segoe UI","Segoe UI Emoji","Segoe UI Symbol",Lato,"Helvetica Neue",Helvetica,Arial,sans-serif">I think it's probably better to handle each known case individually, and error out for values we don't expect.<br></span>"""<br><br>Right now we just handle everything that LLVM directly supports (or at least has an enum for). This ensures we fail on any index that might not be handled correctly. We don't have a counterexample to show that this is the case however. We also don't have proof that no such case exists. This is airing on the side of caution at the cost of having architecture specific enum usage outside of <span style="color:rgb(33,33,33)">architecture-specific code. No one, my self included, seems to really know what's best here.<br><br>Maybe this is a discussion for llvm-dev?</span></div><div class="m_4152240823519784738elided-text"><br><div class="gmail_quote"><div dir="ltr">On Mon, Sep 11, 2017 at 2:44 PM Rafael Avila de Espindola via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Petr Hosek via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> writes:<br>
<br>
> Author: phosek<br>
> Date: Thu Sep 7 16:02:50 2017<br>
> New Revision: 312756<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=312756&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=312756&view=rev</a><br>
> Log:<br>
> [llvm-objcopy] Add support for special section indexes in symbol table greater than SHN_LORESERVE<br>
><br>
> As is indexes above SHN_LORESERVE will not be handled correctly because<br>
> they'll be treated as indexes of sections rather than special values<br>
> that should just be copied. This change adds support to copy them<br>
> though.<br>
><br>
> Patch by Jake Ehrlich<br>
><br>
> Differential Revision: <a href="https://reviews.llvm.org/D37393" rel="noreferrer" target="_blank">https://reviews.llvm.org/D37393</a><br>
><br>
> Added:<br>
> llvm/trunk/test/tools/llvm-objcopy/abs-symbol.test<br>
> llvm/trunk/test/tools/llvm-objcopy/common-symbol.test<br>
> llvm/trunk/test/tools/llvm-objcopy/section-index-unsupported.test<br>
> Modified:<br>
> llvm/trunk/tools/llvm-objcopy/Object.cpp<br>
> llvm/trunk/tools/llvm-objcopy/Object.h<br>
><br>
> Added: llvm/trunk/test/tools/llvm-objcopy/abs-symbol.test<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objcopy/abs-symbol.test?rev=312756&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objcopy/abs-symbol.test?rev=312756&view=auto</a><br>
> ==============================================================================<br>
> --- llvm/trunk/test/tools/llvm-objcopy/abs-symbol.test (added)<br>
> +++ llvm/trunk/test/tools/llvm-objcopy/abs-symbol.test Thu Sep 7 16:02:50 2017<br>
> @@ -0,0 +1,36 @@<br>
> +# RUN: yaml2obj %s > %t<br>
> +# RUN: llvm-objcopy %t %t2<br>
> +# RUN: llvm-readobj -symbols %t2 | FileCheck %s<br>
> +<br>
> +!ELF<br>
> +FileHeader:<br>
> + Class: ELFCLASS64<br>
> + Data: ELFDATA2LSB<br>
> + Type: ET_EXEC<br>
> + Machine: EM_X86_64<br>
> +Symbols:<br>
> + Global:<br>
> + - Name: test<br>
> + Index: SHN_ABS<br>
> + Value: 0x1234<br>
> +<br>
> +#CHECK: Symbols [<br>
> +#CHECK-NEXT: Symbol {<br>
> +#CHECK-NEXT: Name:<br>
> +#CHECK-NEXT: Value: 0x0<br>
> +#CHECK-NEXT: Size: 0<br>
> +#CHECK-NEXT: Binding: Local (0x0)<br>
> +#CHECK-NEXT: Type: None (0x0)<br>
> +#CHECK-NEXT: Other: 0<br>
> +#CHECK-NEXT: Section: Undefined (0x0)<br>
> +#CHECK-NEXT: }<br>
> +#CHECK-NEXT: Symbol {<br>
> +#CHECK-NEXT: Name: test<br>
> +#CHECK-NEXT: Value: 0x1234<br>
> +#CHECK-NEXT: Size: 0<br>
> +#CHECK-NEXT: Binding: Global (0x1)<br>
> +#CHECK-NEXT: Type: None (0x0)<br>
> +#CHECK-NEXT: Other: 0<br>
> +#CHECK-NEXT: Section: Absolute (0xFFF1)<br>
> +#CHECK-NEXT: }<br>
> +#CHECK-NEXT:]<br>
><br>
> Added: llvm/trunk/test/tools/llvm-objcopy/common-symbol.test<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objcopy/common-symbol.test?rev=312756&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objcopy/common-symbol.test?rev=312756&view=auto</a><br>
> ==============================================================================<br>
> --- llvm/trunk/test/tools/llvm-objcopy/common-symbol.test (added)<br>
> +++ llvm/trunk/test/tools/llvm-objcopy/common-symbol.test Thu Sep 7 16:02:50 2017<br>
> @@ -0,0 +1,84 @@<br>
> +# RUN: yaml2obj %s > %t<br>
> +# RUN: llvm-objcopy %t %t2<br>
> +# RUN: llvm-readobj -symbols %t2 | FileCheck %s<br>
> +<br>
> +!ELF<br>
> +FileHeader:<br>
> + Class: ELFCLASS64<br>
> + Data: ELFDATA2LSB<br>
> + Type: ET_EXEC<br>
> + Machine: EM_X86_64<br>
> +Symbols:<br>
> + Global:<br>
> + - Name: test<br>
> + Index: SHN_COMMON<br>
> + Value: 0x1234<br>
> + - Name: test2<br>
> + Index: SHN_HEXAGON_SCOMMON<br>
> + Value: 0x1235<br>
> + - Name: test3<br>
> + Index: SHN_HEXAGON_SCOMMON_2<br>
> + Value: 0x1236<br>
> + - Name: test4<br>
> + Index: SHN_HEXAGON_SCOMMON_4<br>
> + Value: 0x1237<br>
> + - Name: test5<br>
> + Index: SHN_HEXAGON_SCOMMON_8<br>
> + Value: 0x1238<br>
> +<br>
> +#CHECK: Symbols [<br>
> +#CHECK-NEXT: Symbol {<br>
> +#CHECK-NEXT: Name:<br>
> +#CHECK-NEXT: Value: 0x0<br>
> +#CHECK-NEXT: Size: 0<br>
> +#CHECK-NEXT: Binding: Local (0x0)<br>
> +#CHECK-NEXT: Type: None (0x0)<br>
> +#CHECK-NEXT: Other: 0<br>
> +#CHECK-NEXT: Section: Undefined (0x0)<br>
> +#CHECK-NEXT: }<br>
> +#CHECK-NEXT: Symbol {<br>
> +#CHECK-NEXT: Name: test<br>
> +#CHECK-NEXT: Value: 0x1234<br>
> +#CHECK-NEXT: Size: 0<br>
> +#CHECK-NEXT: Binding: Global (0x1)<br>
> +#CHECK-NEXT: Type: None (0x0)<br>
> +#CHECK-NEXT: Other: 0<br>
> +#CHECK-NEXT: Section: Common (0xFFF2)<br>
> +#CHECK-NEXT: }<br>
> +#CHECK-NEXT: Symbol {<br>
> +#CHECK-NEXT: Name: test2<br>
> +#CHECK-NEXT: Value: 0x1235<br>
> +#CHECK-NEXT: Size: 0<br>
> +#CHECK-NEXT: Binding: Global (0x1)<br>
> +#CHECK-NEXT: Type: None (0x0)<br>
> +#CHECK-NEXT: Other: 0<br>
> +#CHECK-NEXT: Section: Processor Specific (0xFF00)<br>
> +#CHECK-NEXT: }<br>
> +#CHECK-NEXT: Symbol {<br>
> +#CHECK-NEXT: Name: test3<br>
> +#CHECK-NEXT: Value: 0x1236<br>
> +#CHECK-NEXT: Size: 0<br>
> +#CHECK-NEXT: Binding: Global (0x1)<br>
> +#CHECK-NEXT: Type: None (0x0)<br>
> +#CHECK-NEXT: Other: 0<br>
> +#CHECK-NEXT: Section: Processor Specific (0xFF02)<br>
> +#CHECK-NEXT: }<br>
> +#CHECK-NEXT: Symbol {<br>
> +#CHECK-NEXT: Name: test4<br>
> +#CHECK-NEXT: Value: 0x1237<br>
> +#CHECK-NEXT: Size: 0<br>
> +#CHECK-NEXT: Binding: Global (0x1)<br>
> +#CHECK-NEXT: Type: None (0x0)<br>
> +#CHECK-NEXT: Other: 0<br>
> +#CHECK-NEXT: Section: Processor Specific (0xFF03)<br>
> +#CHECK-NEXT: }<br>
> +#CHECK-NEXT: Symbol {<br>
> +#CHECK-NEXT: Name: test5<br>
> +#CHECK-NEXT: Value: 0x1238<br>
> +#CHECK-NEXT: Size: 0<br>
> +#CHECK-NEXT: Binding: Global (0x1)<br>
> +#CHECK-NEXT: Type: None (0x0)<br>
> +#CHECK-NEXT: Other: 0<br>
> +#CHECK-NEXT: Section: Processor Specific (0xFF04)<br>
> +#CHECK-NEXT: }<br>
> +#CHECK-NEXT:]<br>
><br>
> Added: llvm/trunk/test/tools/llvm-objcopy/section-index-unsupported.test<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objcopy/section-index-unsupported.test?rev=312756&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objcopy/section-index-unsupported.test?rev=312756&view=auto</a><br>
> ==============================================================================<br>
> --- llvm/trunk/test/tools/llvm-objcopy/section-index-unsupported.test (added)<br>
> +++ llvm/trunk/test/tools/llvm-objcopy/section-index-unsupported.test Thu Sep 7 16:02:50 2017<br>
> @@ -0,0 +1,15 @@<br>
> +# RUN: yaml2obj %s > %t<br>
> +# RUN: not llvm-objcopy %t %t2 2>&1 >/dev/null | FileCheck %s<br>
> +<br>
> +!ELF<br>
> +FileHeader:<br>
> + Class: ELFCLASS64<br>
> + Data: ELFDATA2LSB<br>
> + Type: ET_EXEC<br>
> + Machine: EM_X86_64<br>
> +Symbols:<br>
> + Global:<br>
> + - Name: test<br>
> + Index: 0xff05<br>
> +<br>
> +# CHECK: [[_:.*]] Symbol 'test' has unsupported value greater than or equal to SHN_LORESERVE: 65285<br>
><br>
> Modified: llvm/trunk/tools/llvm-objcopy/Object.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objcopy/Object.cpp?rev=312756&r1=312755&r2=312756&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objcopy/Object.cpp?rev=312756&r1=312755&r2=312756&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/tools/llvm-objcopy/Object.cpp (original)<br>
> +++ llvm/trunk/tools/llvm-objcopy/Object.cpp Thu Sep 7 16:02:50 2017<br>
> @@ -90,14 +90,54 @@ void StringTableSection::writeSection(Fi<br>
> StrTabBuilder.write(Out.getBufferStart() + Offset);<br>
> }<br>
><br>
> +static bool isValidReservedSectionIndex(uint16_t Index) {<br>
> + switch (Index) {<br>
> + case SHN_ABS:<br>
> + case SHN_COMMON:<br>
> + case SHN_HEXAGON_SCOMMON:<br>
> + case SHN_HEXAGON_SCOMMON_2:<br>
> + case SHN_HEXAGON_SCOMMON_4:<br>
> + case SHN_HEXAGON_SCOMMON_8:<br>
> + return true;<br>
> + default:<br>
> + return false;<br>
> + }<br>
> +}<br>
<br>
Should this function just be "Index >= SHN_LORESERVE"? It is strange to<br>
check for Hexagon values in non architecture-specific code. It also<br>
looks like you just need to check for "is this really an section<br>
index", which is what SHN_LORESERVE provides.<br>
<br>
Cheers,<br>
Rafael<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>
</div><br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>
</blockquote></div></div>