[PATCH] MCJIT honor symbol visibility settings when populating the global symbol table

[Anthony Pesch]

Thanks for the fast review.

I'm out of the country currently, but I'll update this again when I get
back in ~2 weeks.

- Anthony
On Sep 2, 2014 1:29 PM, "Rafael Espíndola" <rafael.espindola at gmail.com>
wrote:

> This looks wrong:
>
> +  if ((ESym->getVisibility() == ELF::STV_DEFAULT &&
> +      (ESym->getBinding() == ELF::STB_GLOBAL || ESym->getBinding() ==
> ELF::STB_WEAK)) ||
> +      ESym->getVisibility() == ELF::STV_PROTECTED)
> +    Result |= SymbolRef::SF_Exported;
>
> You might wast to use a predicate function to make this trivial to read:
>
> static bool isExportedToOtherDSO(...) {
> if (Binding != STB_GLOBAL && Binding != STB_WEAK)
>   return false;
> if (Visibility != STV_DEFAULT && Visibility != STV_PROTECTED)
>   return false
> return true;
> }
>
> -  unsigned char st_other; // Must be zero; reserved
> +  unsigned char st_other; // Symbol's visibility attributes
>
> The old comment was wrong, but the new one is incomplete. The
> visibility is the lower 2 bits, the rest is zero.
>
> +    SF_Exported = 1U << 6,       // Symbol is visible to other
> dynamic symbol objects
>
> DSO is Dynamic Shared Object
>
>
>
>
> On 29 August 2014 17:07, Anthony Pesch <inolen at gmail.com> wrote:
> > I've updated the patch to reflect Rafael's comments.
> >
> > Also, as we discussed Lang, I've added .globl directives for the
> > symbols used in the MachO tests that were breaking.
> >
> > Sorry in advance for the added noise in the test diffs. The tests I
> > patched had some mixed white spacing, so I converted them to tabs as
> > that seemed to be more common in those particular files.
> >
> >  - Anthony
> >
> > On Fri, Aug 22, 2014 at 1:54 PM, Rafael Espíndola
> > <rafael.espindola at gmail.com> wrote:
> >> if (ESym->getVisibility() == ELF::STV_DEFAULT &&
> >> +      (ESym->getBinding() == ELF::STB_GLOBAL || ESym->getBinding() ==
> >> ELF::STB_WEAK))
> >> +    Result |= SymbolRef::SF_Exported;
> >>
> >> ELF::STV_PROTECTED should also be exported.
> >>
> >>
> >>
> >> On 22 August 2014 16:15, Lang Hames <lhames at gmail.com> wrote:
> >>> The RuntimeDyld side of this looks good to me.
> >>>
> >>> Rafael - I'd appreciate your take on the ELF changes. If the "st_other"
> >>> field in Elf_Sym_Base is going to be used solely for visibility (as it
> seems
> >>> in this patch), should it be renamed to "st_visibility" ?
> >>
> >> No, st_other is the name used in the spec. The low 2 bits are the
> >> visibility, the others are 0. Maybe expand the comment a bit?
> >>
> >> +  unsigned char getVisibility() const { return st_other; }
> >>
> >> Add a " & 3 " to get only the low bits. If you want, assert that the
> >> high bits are zero.
> >>
> >> +    SF_Exported = 1U << 6,       // Symbol is visible by other modules
> >>
> >> Modules is ambiguous.  A hidden symbol is visible from another .o. DSO
> >> is probably a better description.
> >>
> >>
> >> + void setVisibility(unsigned char v) { st_other = v; }
> >>
> >> assert that v has only two bits set. Only set the last two bits or
> >> assert the rest is zero.
> >>
> >> Cheers,
> >> Rafael
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140902/2c884bc5/attachment.html>