<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Mar 14, 2015 at 6:15 PM, Davide Italiano <span dir="ltr"><<a href="mailto:davide@freebsd.org" target="_blank">davide@freebsd.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Sat, Mar 14, 2015 at 3:54 PM, Shankar Kalpathi Easwaran<br>
<<a href="mailto:shankarke@gmail.com">shankarke@gmail.com</a>> wrote:<br>
> Forgot to mention about your previous change, that you need to  add changes to ReaderWriterYAML(YAML Testing) and ReaderNative and WriterNative(Native formats) too.<br>
><br>
> Adding other reviewers here.<br>
><br>
<br>
</span>OK, thanks for letting me know. I'll take care of that the other two formats.<br>
I'm not familiar with them so it might take some time.<br>
<span class=""><br></span></blockquote><div>Sure.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
><br>
> ================<br>
> Comment at: lib/ReaderWriter/ELF/SectionChunks.h:790<br>
> @@ -776,1 +789,3 @@<br>
> +  }<br>
> +  sym.setVisibility(visibility);<br>
>    sym.setBindingAndType(binding, type);<br>
> ----------------<br>
> 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 ?<br>
><br>
<br>
</span>To me the whole notion of modeling visibility using 'Scope' is<br>
confusing. I don't see why we can't expose visibility as I did in my<br>
previous patch </blockquote><div><br></div><div> 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.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
><br>
> ================<br>
> Comment at: lib/ReaderWriter/ELF/SectionChunks.h:804<br>
> @@ -787,2 +803,3 @@<br>
> +    // the other bits of st_other.<br>
>      sym.st_other = llvm::ELF::STV_HIDDEN;<br>
>      binding = llvm::ELF::STB_LOCAL;<br>
> ----------------<br>
> could this be sym.st_other = sym.st_other & ~0x3 |llvm::ELF::STV_HIDDEN ?<br>
><br>
<br>
</span>I attempted to fix these (and others) in r232282.<br>
<span class=""><br>
> ================<br>
> Comment at: lib/ReaderWriter/ELF/SectionChunks.h:814<br>
> @@ -796,1 +813,3 @@<br>
> +  case AbsoluteAtom::scopeProtected:<br>
> +    break;<br>
>    }<br>
> ----------------<br>
> assert<br>
><br>
<br>
</span>OK.<br>
</blockquote></div><br></div></div>