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

Fangrui Song via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Aug 27 03:13:01 PDT 2019


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());
----------------
jhenderson wrote:
> 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.
Allow all of the following?

```
Other: [ STV_HIDDEN, STO_MIPS_OPTIONAL ] # MIPS
Other: [ STV_HIDDEN, 192 ] # PPC64 ELFv2
Other: 2
```

I think that will be great.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66642





More information about the llvm-commits mailing list