<div dir="ltr">I've reverted this change, it was causing check-llvm failures on the MSan bot: <a href="http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/34675/steps/check-lld%20msan/logs/stdio">http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/34675/steps/check-lld%20msan/logs/stdio</a></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Aug 27, 2019 at 2:57 AM George Rimar via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: grimar<br>
Date: Tue Aug 27 02:58:39 2019<br>
New Revision: 370032<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=370032&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=370032&view=rev</a><br>
Log:<br>
[yaml2obj] - Don't allow setting StOther and Other/Visibility at the same time.<br>
<br>
This is a follow up discussed in the comments of D66583.<br>
<br>
Currently, if for example, we have both StOther and Other set in YAML document for a symbol,<br>
then yaml2obj reports an "unknown key 'Other'" error.<br>
It happens because 'mapOptional()' is never called for 'Other/Visibility' in this case,<br>
leaving those unhandled.<br>
<br>
This message does not describe the reason of the error well. This patch fixes it.<br>
<br>
Differential revision: <a href="https://reviews.llvm.org/D66642" rel="noreferrer" target="_blank">https://reviews.llvm.org/D66642</a><br>
<br>
Modified:<br>
llvm/trunk/include/llvm/ObjectYAML/ELFYAML.h<br>
llvm/trunk/lib/ObjectYAML/ELFEmitter.cpp<br>
llvm/trunk/lib/ObjectYAML/ELFYAML.cpp<br>
llvm/trunk/test/tools/yaml2obj/elf-symbol-stother.yaml<br>
<br>
Modified: llvm/trunk/include/llvm/ObjectYAML/ELFYAML.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ObjectYAML/ELFYAML.h?rev=370032&r1=370031&r2=370032&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ObjectYAML/ELFYAML.h?rev=370032&r1=370031&r2=370032&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/ObjectYAML/ELFYAML.h (original)<br>
+++ llvm/trunk/include/llvm/ObjectYAML/ELFYAML.h Tue Aug 27 02:58:39 2019<br>
@@ -107,7 +107,12 @@ struct Symbol {<br>
ELF_STB Binding;<br>
llvm::yaml::Hex64 Value;<br>
llvm::yaml::Hex64 Size;<br>
- uint8_t Other;<br>
+ Optional<uint8_t> Other;<br>
+<br>
+ // This can be used to set any custom value for the st_other field<br>
+ // when it is not possible to do so using the "Other" field, which only takes<br>
+ // specific named constants.<br>
+ Optional<uint8_t> StOther;<br>
};<br>
<br>
struct SectionOrType {<br>
<br>
Modified: llvm/trunk/lib/ObjectYAML/ELFEmitter.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ObjectYAML/ELFEmitter.cpp?rev=370032&r1=370031&r2=370032&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ObjectYAML/ELFEmitter.cpp?rev=370032&r1=370031&r2=370032&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/ObjectYAML/ELFEmitter.cpp (original)<br>
+++ llvm/trunk/lib/ObjectYAML/ELFEmitter.cpp Tue Aug 27 02:58:39 2019<br>
@@ -464,7 +464,12 @@ toELFSymbols(NameToIdxMap &SN2I, ArrayRe<br>
}<br>
// else Symbol.st_shndex == SHN_UNDEF (== 0), since it was zero'd earlier.<br>
Symbol.st_value = Sym.Value;<br>
- Symbol.st_other = Sym.Other;<br>
+<br>
+ if (Sym.Other)<br>
+ Symbol.st_other = *Sym.Other;<br>
+ else if (Sym.StOther)<br>
+ Symbol.st_other = *Sym.StOther;<br>
+<br>
Symbol.st_size = Sym.Size;<br>
}<br>
<br>
<br>
Modified: llvm/trunk/lib/ObjectYAML/ELFYAML.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ObjectYAML/ELFYAML.cpp?rev=370032&r1=370031&r2=370032&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ObjectYAML/ELFYAML.cpp?rev=370032&r1=370031&r2=370032&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/ObjectYAML/ELFYAML.cpp (original)<br>
+++ llvm/trunk/lib/ObjectYAML/ELFYAML.cpp Tue Aug 27 02:58:39 2019<br>
@@ -866,15 +866,28 @@ void MappingTraits<ELFYAML::ProgramHeade<br>
namespace {<br>
<br>
struct NormalizedOther {<br>
- NormalizedOther(IO &)<br>
- : Visibility(ELFYAML::ELF_STV(0)), Other(ELFYAML::ELF_STO(0)) {}<br>
- NormalizedOther(IO &, uint8_t Original)<br>
- : Visibility(Original & 0x3), Other(Original & ~0x3) {}<br>
+ NormalizedOther(IO &) {}<br>
+ NormalizedOther(IO &, Optional<uint8_t> Original) {<br>
+ if (uint8_t Val = *Original & 0x3)<br>
+ Visibility = Val;<br>
+ if (uint8_t Val = *Original & ~0x3)<br>
+ Other = Val;<br>
+ }<br>
<br>
- uint8_t denormalize(IO &) { return Visibility | Other; }<br>
+ Optional<uint8_t> denormalize(IO &) {<br>
+ if (!Visibility && !Other)<br>
+ return None;<br>
+<br>
+ uint8_t Ret = 0;<br>
+ if (Visibility)<br>
+ Ret |= *Visibility;<br>
+ if (Other)<br>
+ Ret |= *Other;<br>
+ return Ret;<br>
+ }<br>
<br>
- ELFYAML::ELF_STV Visibility;<br>
- ELFYAML::ELF_STO Other;<br>
+ Optional<ELFYAML::ELF_STV> Visibility;<br>
+ Optional<ELFYAML::ELF_STO> Other;<br>
};<br>
<br>
} // end anonymous namespace<br>
@@ -896,17 +909,16 @@ void MappingTraits<ELFYAML::Symbol>::map<br>
// targets (e.g. MIPS) may use it to specify the named bits to set (e.g.<br>
// STO_MIPS_OPTIONAL). For producing broken objects we want to allow writing<br>
// any value to st_other. To do this we allow one more field called "StOther".<br>
- // If it is present in a YAML document, we set st_other to that integer,<br>
- // ignoring the other fields.<br>
- Optional<llvm::yaml::Hex64> Other;<br>
- IO.mapOptional("StOther", Other);<br>
- if (Other) {<br>
- Symbol.Other = *Other;<br>
- } else {<br>
- MappingNormalization<NormalizedOther, uint8_t> Keys(IO, Symbol.Other);<br>
- IO.mapOptional("Visibility", Keys->Visibility, ELFYAML::ELF_STV(0));<br>
- IO.mapOptional("Other", Keys->Other, ELFYAML::ELF_STO(0));<br>
- }<br>
+ // If it is present in a YAML document, we set st_other to its integer value<br>
+ // whatever it is.<br>
+ // obj2yaml should not print 'StOther', it should print 'Visibility' and<br>
+ // 'Other' fields instead.<br>
+ assert(!IO.outputting() || !Symbol.StOther.hasValue());<br>
+ IO.mapOptional("StOther", Symbol.StOther);<br>
+ MappingNormalization<NormalizedOther, Optional<uint8_t>> Keys(IO,<br>
+ Symbol.Other);<br>
+ IO.mapOptional("Visibility", Keys->Visibility);<br>
+ IO.mapOptional("Other", Keys->Other);<br>
}<br>
<br>
StringRef MappingTraits<ELFYAML::Symbol>::validate(IO &IO,<br>
@@ -915,6 +927,8 @@ StringRef MappingTraits<ELFYAML::Symbol><br>
return "Index and Section cannot both be specified for Symbol";<br>
if (Symbol.NameIndex && !Symbol.Name.empty())<br>
return "Name and NameIndex cannot both be specified for Symbol";<br>
+ if (Symbol.StOther && Symbol.Other)<br>
+ return "StOther cannot be specified for Symbol with either Visibility or Other";<br>
return StringRef();<br>
}<br>
<br>
<br>
Modified: llvm/trunk/test/tools/yaml2obj/elf-symbol-stother.yaml<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/yaml2obj/elf-symbol-stother.yaml?rev=370032&r1=370031&r2=370032&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/yaml2obj/elf-symbol-stother.yaml?rev=370032&r1=370031&r2=370032&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/tools/yaml2obj/elf-symbol-stother.yaml (original)<br>
+++ llvm/trunk/test/tools/yaml2obj/elf-symbol-stother.yaml Tue Aug 27 02:58:39 2019<br>
@@ -77,3 +77,32 @@ Symbols:<br>
StOther: 4<br>
- Name: bar<br>
StOther: 0xff<br>
+<br>
+## Check we can't set StOther for a symbol if Visibility or Other is also specified.<br>
+<br>
+# RUN: not yaml2obj --docnum=5 2>&1 %s | FileCheck %s --check-prefix=ERR2<br>
+# RUN: not yaml2obj --docnum=6 2>&1 %s | FileCheck %s --check-prefix=ERR2<br>
+<br>
+# ERR2: error: StOther cannot be specified for Symbol with either Visibility or Other<br>
+<br>
+--- !ELF<br>
+FileHeader:<br>
+ Class: ELFCLASS32<br>
+ Data: ELFDATA2LSB<br>
+ Type: ET_REL<br>
+ Machine: EM_MIPS<br>
+Symbols:<br>
+ - Name: foo<br>
+ StOther: 0<br>
+ Other: [ STO_MIPS_OPTIONAL ]<br>
+<br>
+--- !ELF<br>
+FileHeader:<br>
+ Class: ELFCLASS32<br>
+ Data: ELFDATA2LSB<br>
+ Type: ET_REL<br>
+ Machine: EM_MIPS<br>
+Symbols:<br>
+ - Name: foo<br>
+ StOther: 0<br>
+ Visibility: STV_DEFAULT<br>
<br>
<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="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>