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