[Mesa-stable] [Mesa-dev] [PATCH] opencl: autotools: Fix linking order for OpenCL target

[Jan Vesely]

On Wed, 2018-05-02 at 18:38 +0200, Kai Wasserbäch wrote:
> Hey Jan,
> Jan Vesely wrote on 01.05.2018 23:59:
> > On Tue, 2018-05-01 at 18:23 +0200, Kai Wasserbäch wrote:
> > > Jan Vesely wrote on 01.05.2018 17:19:
> > > > On Tue, 2018-05-01 at 14:14 +0200, Kai Wasserbäch wrote:
> > > > > [...]
> > > > > 
> > > > >  src/gallium/targets/opencl/Makefile.am | 3 +--
> > > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/src/gallium/targets/opencl/Makefile.am b/src/gallium/targets/opencl/Makefile.am
> > > > > index de68a93ad5..f0e1de7797 100644
> > > > > --- a/src/gallium/targets/opencl/Makefile.am
> > > > > +++ b/src/gallium/targets/opencl/Makefile.am
> > > > > @@ -23,11 +23,10 @@ lib at OPENCL_LIBNAME@_la_LIBADD = \
> > > > >  	$(LIBELF_LIBS) \
> > > > >  	$(DLOPEN_LIBS) \
> > > > >  	-lclangCodeGen \
> > > > > -	-lclangFrontendTool \
> > > > >  	-lclangFrontend \
> > > > > +	-lclangFrontendTool \
> > > > 
> > > > This is strange. Why does reordering help here? Do we use -Wl,--as-
> > > > needed anywhere?
> > > 
> > > No, not that I can see.
> > > 
> > > > Should we use -Wl,--start-group/-Wl,--end-group for all clang libraries
> > > > instead?
> > > 
> > > Maybe? This was the simplest fix I could come up with, but if there's a
> > > preference for a link group, I can give that a try as well.
> > 
> > So the fix is to change ordering?
> 
> yes.
> 
> > Does using groups fix the issue as well? I think that would be
> > preferable, but I use split .so files, so I don't hit this issue.
> 
> I tried convincing autotools to work with those flags but failed. The only
> option I see to solve this, is very messy IMHO (and would still need the
> ordering fix): putting -Wl,--{start,end}-group directly into the right places in
> lib at OPENCL_LIBNAME@_la_LIBADD is forbidden by automake ("error: linker flags
> such as '-Wl,--start-group' belong in 'lib at OPENCL_LIBNAME@_la_LDFLAGS'") and
> adding them to lib at OPENCL_LIBNAME@_la_LDFLAGS like automake is suggesting won't
> work for obvious reasons. The only solution I can see is to work with
> substitution because automake seems to "not see" the flags then. I could do an
> unconditional replacement, but there are probably linkers with no support for
> these flags, which would mean I'd have to do the ordering fix in any case and
> then conditionally set "-Wl,--{start,end}-group" just for the GNU toolchain with
> no immediate benefit beyond future-proofing this section.
> But maybe people who are deeper into the whole autotools stuff (Emil?
> Francisco?) can point me to a solution? Otherwise I'd like to return to my
> original patch which fixes the FTBFS and works for now. Or maybe the library
> could be linked against libclang.so (at least when --enable-llvm-shared-libs is set?

Thank for looking into this. We probably need CLANG_LIBS handling
similar to LLVM_LIBS. I agree this is the best fix for now.

Acked-by: Jan Vesely <jan.vesely at rutgers.edu>

libclang.so might be a solkution, but I'm not sure how it interacts
with older or static build clang. It's also weird that we are linking
to clang here instead of clover which actually uses clang symbols.

@Emil, are you OK with this patch?

> 
> > > > >  	-lclangDriver \
> > > > >  	-lclangSerialization \
> > > > > -	-lclangCodeGen \
> > > > 
> > > > Is this change related?
> > > 
> > > Not really, just a minor clean-up while I was busy a few lines above.
> > > "clangCodeGen" is already named on the first Clang library line.
> > 
> > ah, all right, maybe mention it in the commit message?
> 
> Do I need to resend the patch for that or can you just add a line like "This
> change also removes the duplicate clangCodeGen line (trivial change)." before
> pushing, considering, that there are two T-b tags to be added anyway?

I'll add it on my side before pushing the patch.

thanks,
Jan

> 
> Cheers,
> Kai
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/mesa-stable/attachments/20180502/8849baa7/attachment-0001.sig>