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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
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());
----------------
grimar wrote:
> 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?
I agree that dumping raw `StOther` is probably the best for PPC64 (its ELFv2 ABI uses the high bits)...


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

https://reviews.llvm.org/D66642