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

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM.



================
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());
----------------
MaskRay wrote:
> 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)...
Okay, I agree that it's a possible future extension to obj2yaml to handle that case, as long as it's not going to produce incorrect ELF objects (e.g. with a value other than requested) in the case where it doesn't work (I'm okay with it emitting an error in this case for now). Using a raw `StOther` seems like the right thing to do longer term when things don't work.

A slightly more radical solution could be to only have one field "Other" (i.e. no Visibility or StOther), which simply takes a list of named parameters or an arbitrary integer. STV_HIDDEN etc could be allowed via the named parameters. I don't know if this is a good idea or not though.


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

https://reviews.llvm.org/D66642