[PATCH] D66642: [yaml2obj] - Don't allow setting StOther and Other/Visibility at the same time.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: include/llvm/ObjectYAML/ELFYAML.h:112
+
+  // This can be used to set any custom value for the sh_other field
+  // when it is not possible to do using the "Other" field because of need to
----------------
sh_other -> st_other


================
Comment at: include/llvm/ObjectYAML/ELFYAML.h:113-115
+  // when it is not possible to do using the "Other" field because of need to
+  // use unnamed constants, an arbitrary or simply broken values that would
+  // not be accepted otherwise as a regular bit flags.
----------------
I don't think you need the majority of this comment. How about

"when it is not possible to do so using the "Other" field, which only takes specific named constants".


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:471
+    else if (Sym.StOther)
+      Symbol.st_other = *Sym.StOther;
+
----------------
I just want to be clear that st_other is zero-initialized?


================
Comment at: lib/ObjectYAML/ELFYAML.cpp:914-915
+  // whatever it is.
+  // obj2yaml should not print 'StOther', it should print 'Visibility' and
+  // 'Other' fields instead.
+  assert(!IO.outputting() || !Symbol.StOther.hasValue());
----------------
What if it's not possible to represent the value of st_other using Visibility and Other?


================
Comment at: lib/ObjectYAML/ELFYAML.cpp:931
+  if (Symbol.StOther && Symbol.Other)
+    return "StOther and Visibility/Other cannot both be specified for Symbol";
   return StringRef();
----------------
Maybe "StOther cannot be specified for Symbol with either Visibility or Other"?


================
Comment at: test/tools/yaml2obj/elf-symbol-stother.yaml:81
+
+## Check we can't set StOther with Visibility/Other for a symbol at the same time.
+
----------------
How about "Check we can't set StOther for a symbol if Visibility or Other is also specified."


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66642/new/

https://reviews.llvm.org/D66642