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

[Jim Grosbach]

> On Aug 29, 2014, at 2:07 PM, 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.

Should be the other way around, actually. Spaces are preferred, not tabs, including for test cases where they are otherwise not significant. We haven’t historically been as diligent about that as we should in .s test cases, unfortunately.

-Jim

> 
> - Anthony
> 
> On Fri, Aug 22, 2014 at 1:54 PM, Rafael Espíndola
> <rafael.espindola at gmail.com <mailto: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
> <symvis2.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140926/95d83cb1/attachment.html>