[PATCH] [ELF] Initial support for symbols visibility

[Davide Italiano]

On Sat, Mar 14, 2015 at 3:54 PM, Shankar Kalpathi Easwaran
<shankarke at gmail.com> wrote:
> Forgot to mention about your previous change, that you need to  add changes to ReaderWriterYAML(YAML Testing) and ReaderNative and WriterNative(Native formats) too.
>
> Adding other reviewers here.
>

OK, thanks for letting me know. I'll take care of that the other two formats.
I'm not familiar with them so it might take some time.

>
> ================
> Comment at: lib/ReaderWriter/ELF/SectionChunks.h:790
> @@ -776,1 +789,3 @@
> +  }
> +  sym.setVisibility(visibility);
>    sym.setBindingAndType(binding, type);
> ----------------
> 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 ?
>

To me the whole notion of modeling visibility using 'Scope' is
confusing. I don't see why we can't expose visibility as I did in my
previous patch.

>
> ================
> Comment at: lib/ReaderWriter/ELF/SectionChunks.h:804
> @@ -787,2 +803,3 @@
> +    // the other bits of st_other.
>      sym.st_other = llvm::ELF::STV_HIDDEN;
>      binding = llvm::ELF::STB_LOCAL;
> ----------------
> could this be sym.st_other = sym.st_other & ~0x3 |llvm::ELF::STV_HIDDEN ?
>

I attempted to fix these (and others) in r232282.

> ================
> Comment at: lib/ReaderWriter/ELF/SectionChunks.h:814
> @@ -796,1 +813,3 @@
> +  case AbsoluteAtom::scopeProtected:
> +    break;
>    }
> ----------------
> assert
>

OK.