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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
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.
----------------
jhenderson wrote:
> 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".
LG, thanks!


================
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());
----------------
jhenderson wrote:
> What if it's not possible to represent the value of st_other using Visibility and Other?
Yes, this is a right question that I thought about too, but still have no good answer for now.

It is related to obj2yaml, not to yaml2obj and technically the straightforward way to solve this for the common case would probably be
to know somehow if the mapping operation failed or not. Then we can fall back to dumping the field just as "StOther" in some cases.
I think the existent API can't handle that.

There are other options probably. We can perhaps stop trying dumping st_other as flags at all, because: 

PPC64 stores int values in this field, not just flags:

```
// Special values for the st_other field in the symbol table entry for PPC64.
enum {
  STO_PPC64_LOCAL_BIT = 5,
  STO_PPC64_LOCAL_MASK = (7 << STO_PPC64_LOCAL_BIT)
};
static inline int64_t decodePPC64LocalEntryOffset(unsigned Other) {
  unsigned Val = (Other & STO_PPC64_LOCAL_MASK) >> STO_PPC64_LOCAL_BIT;
  return ((1 << Val) >> 2) << 2;
}
static inline unsigned encodePPC64LocalEntryOffset(int64_t Offset) {
  unsigned Val =
      (Offset >= 4 * 4 ? (Offset >= 8 * 4 ? (Offset >= 16 * 4 ? 6 : 5) : 4)
                       : (Offset >= 2 * 4 ? 3 : (Offset >= 1 * 4 ? 2 : 0)));
  return Val << STO_PPC64_LOCAL_BIT;
}
```

So handling st_value as a set of flags is technically not 100% correct for this particular case.
But perhaps we can just try to isolate this corner case (that is inline with my patches at the current step I think).

I need to investigate the possible solutions for obj2yaml a bit deeper to suggest a good approach. Seems it is a subject for a different patch though?


================
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();
----------------
jhenderson wrote:
> Maybe "StOther cannot be specified for Symbol with either Visibility or Other"?
Done.


================
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.
+
----------------
jhenderson wrote:
> How about "Check we can't set StOther for a symbol if Visibility or Other is also specified."
Done.


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

https://reviews.llvm.org/D66642