[PATCH] [ELF] Initial support for symbols visibility

[Shankar Easwaran]

On Sat, Mar 14, 2015 at 6:15 PM, Davide Italiano <davide at freebsd.org> wrote:

> On Sat, Mar 14, 2015 at 3:54 PM, Shankar Kalpathi Easwaran
> <shankarke at gmail.com> wrote:
> > Forgot to mention about your previous change, that you need to  add
> changes to ReaderWriterYAML(YAML Testing) and ReaderNative and
> WriterNative(Native formats) too.
> >
> > Adding other reviewers here.
> >
>
> OK, thanks for letting me know. I'll take care of that the other two
> formats.
> I'm not familiar with them so it might take some time.
>
> Sure.


> >
> > ================
> > Comment at: lib/ReaderWriter/ELF/SectionChunks.h:790
> > @@ -776,1 +789,3 @@
> > +  }
> > +  sym.setVisibility(visibility);
> >    sym.setBindingAndType(binding, type);
> > ----------------
> > This is very confusing, if you see line 771. We set binding for local
> symbols to be STB_LOCAL if scope is set to scopeTranslationUnit, so
> visibility would need to be set as STV_DEFAULT ? No ?
> >
>
> To me the whole notion of modeling visibility using 'Scope' is
> confusing. I don't see why we can't expose visibility as I did in my
> previous patch


 If we want to a new attribute visibility to the Atom, I think we would
need to cleanup and redefine scope IMO. This way it would be much cleaner
than defining visibility attributes in multiple places.



> >
> > ================
> > Comment at: lib/ReaderWriter/ELF/SectionChunks.h:804
> > @@ -787,2 +803,3 @@
> > +    // the other bits of st_other.
> >      sym.st_other = llvm::ELF::STV_HIDDEN;
> >      binding = llvm::ELF::STB_LOCAL;
> > ----------------
> > could this be sym.st_other = sym.st_other & ~0x3 |llvm::ELF::STV_HIDDEN ?
> >
>
> I attempted to fix these (and others) in r232282.
>
> > ================
> > Comment at: lib/ReaderWriter/ELF/SectionChunks.h:814
> > @@ -796,1 +813,3 @@
> > +  case AbsoluteAtom::scopeProtected:
> > +    break;
> >    }
> > ----------------
> > assert
> >
>
> OK.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150314/5dbe4e0a/attachment.html>