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

[Rafael Espíndola]

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