jfb added a comment.

In D60974#1565541 <https://reviews.llvm.org/D60974#1565541>, @plotfi wrote:

> In D60974#1565527 <https://reviews.llvm.org/D60974#1565527>, @jfb wrote:
>
> > In D60974#1565517 <https://reviews.llvm.org/D60974#1565517>, @jfb wrote:
> >
> > > In D60974#1565054 <https://reviews.llvm.org/D60974#1565054>, @plotfi 
> > > wrote:
> > >
> > > > So I think I know what may be going on on your end. The llvm-readelf in 
> > > > your path I believe might be the wrong lllvm-readelf (llvm-readelf-7). 
> > > > Are you sure you built llvm-readelf from git?
> > >
> > >
> > > Please don't do this. Your commit is wrong, and the right action from you 
> > > is to revert it or fix it. I've fixed it for you here: rL364855 
> > > <https://reviews.llvm.org/rL364855>
> >
> >
> > I'm also really not sure this is any good: why does clang look at ELF? In 
> > general I'd expect you to test something *way* earlier than ELF. Are you 
> > sure you're testing the right thing?
>
>
> Ah, I must have mistakenly thought that other tools were already using 
> llvm-readelf in cfe. Sorry about this.
>
> This is in fact intended. The interface stubs feature needs to be verified 
> against the actual symbols that are emitted into the elf binary. In the cases 
> where I use llvm-readelf, I am using it to determine if the symbol is visible 
> (to nm for example) but marked hidden in the .o file (but will end up being 
> hidden in when linked).
>
> Currently clang -emit-interface-stubs is not using any elf libraries, but it 
> needs to verify against llvm-nm and/or llvm-readelf in these lit tests to 
> determine if the visibility of the symbols generated in the text stubs is 
> correct.


`llvm-nm` is already in the list of requirement for clang, so that would be 
fine to use. I'm not sure `llvm-readelf` is necessarily built for all targets, 
so my "fix" might still be broken. It's also another dependency, and a weird 
one at that. Can you test the property you're looking for in IR directly? And 
then test, in LLVM, that the same IR generates the ELF binary you want?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to