[Mesa-dev] [PATCH] Android: Fix LLVM duplicated symbols linking for N and M

[Rob Herring]

On Fri, Aug 18, 2017 at 8:53 PM, Chih-Wei Huang <cwhuang at android-x86.org> wrote:
> 2017-08-19 8:27 GMT+08:00 Emil Velikov <emil.l.velikov at gmail.com>:
>> On 18 August 2017 at 20:46, Rob Herring <robh at kernel.org> wrote:
>>> Both statically linking libLLVMCore and dynamically linking libLLVM causes
>>> duplicated symbols in gallium_dri.so and it fails to dlopen. We don't
>>> really need to link libLLVMCore, but just need generated headers to be
>>> built first. Dynamically linking to libLLVM instead is enough to do
>>> that. Thanks to Qiang Yu for finding the root cause.
>>>
>> Nice find indeed, thanks.
>>
>> This reminds me - a small task for a rainy day.
>> - Wire the version script files into the Android build - see the
>> autoconf snippet below.
>> It will hide the hundreds of symbols when static linking LLVM (aka
>> sidestep the current issue) and make the binary noticeably smaller.
>>
>> gallium_dri_la_LDFLAGS += \
>>        -Wl,--version-script=$(top_srcdir)/src/gallium/targets/dri/dri.sym
>>
>>> With this change, we can align all versions and just have libLLVM as a
>>> shared lib dependency.
>>>
>>> This also requires changes in the M and N versions of LLVM to export the
>>> include paths for libLLVM. AOSP master is okay.
>>>
>> Perfect :-)
>>
>>> Fixes: 26aee6f4d5a ("Android: rework LLVM build support")
>>> Reported-by: Mauro Rossi <issor.oruam at gmail.com>
>>> Cc: Emil Velikov <emil.l.velikov at gmail.com>
>>> Cc: 17.2 <mesa-stable at lists.freedesktop.org>
>>> Signed-off-by: Qiang Yu <Qiang.Yu at amd.com>
>>> Signed-off-by: Rob Herring <robh at kernel.org>
>>
>> Reviewed-by: Emil Velikov <emil.velikov at collabora.com>
>>
>>> ---
>>>  Android.mk                              | 12 ++++--------
>>>  src/amd/Android.common.mk               |  4 +---
>>>  src/gallium/drivers/radeon/Android.mk   |  2 +-
>>>  src/gallium/drivers/radeonsi/Android.mk |  2 +-
>>>  4 files changed, 7 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/Android.mk b/Android.mk
>>> index 6571161c8783..dc4041364551 100644
>>> --- a/Android.mk
>>> +++ b/Android.mk
>>> @@ -92,16 +92,12 @@ define mesa-build-with-llvm
>>>    $(if $(filter $(MESA_ANDROID_MAJOR_VERSION), 4 5), \
>>>      $(warning Unsupported LLVM version in Android $(MESA_ANDROID_MAJOR_VERSION)),) \
>>>    $(if $(filter 6,$(MESA_ANDROID_MAJOR_VERSION)), \
>>> -    $(eval LOCAL_CFLAGS += -DHAVE_LLVM=0x0307 -DMESA_LLVM_VERSION_PATCH=0) \
>>> -    $(eval LOCAL_STATIC_LIBRARIES += libLLVMCore) \
>>> -    $(eval LOCAL_C_INCLUDES += external/llvm/include external/llvm/device/include),) \
>>> +    $(eval LOCAL_CFLAGS += -DHAVE_LLVM=0x0307 -DMESA_LLVM_VERSION_PATCH=0),) \
>
> Hmm, why do we need an extra comma?
> Does it correspond to the else case of $(if ...)?
> If so it could be omitted.

Indeed.

>
>>>    $(if $(filter 7,$(MESA_ANDROID_MAJOR_VERSION)), \
>>> -    $(eval LOCAL_CFLAGS += -DHAVE_LLVM=0x0308 -DMESA_LLVM_VERSION_PATCH=0) \
>>> -    $(eval LOCAL_STATIC_LIBRARIES += libLLVMCore) \
>>> -    $(eval LOCAL_C_INCLUDES += external/llvm/include external/llvm/device/include),) \
>>> +    $(eval LOCAL_CFLAGS += -DHAVE_LLVM=0x0308 -DMESA_LLVM_VERSION_PATCH=0),) \
>>>    $(if $(filter O,$(MESA_ANDROID_MAJOR_VERSION)), \
>>> -    $(eval LOCAL_CFLAGS += -DHAVE_LLVM=0x0309 -DMESA_LLVM_VERSION_PATCH=0) \
>>> -    $(eval LOCAL_HEADER_LIBRARIES += llvm-headers),)
>>> +    $(eval LOCAL_CFLAGS += -DHAVE_LLVM=0x0309 -DMESA_LLVM_VERSION_PATCH=0),) \
>>> +  $(eval LOCAL_SHARED_LIBRARIES += libLLVM)
>> Am I the only person getting tad confused by amount of brackets?
>> As mentioned by Chih-Wei - a shell switch is not possible, but how
>> about a test vague like the following?
>>
>> test "x$(MESA_ANDROID_MAJOR_VERSION)" = "xO" &&
>>    $(eval LOCAL_CFLAGS += -DHAVE_LLVM=0x0309 -DMESA_LLVM_VERSION_PATCH=0)
>
> Only possible if you put it into $(shell ...)
> That gives me an idea. Maybe we ca do like
>
> $(shell case "$(MESA_ANDROID_MAJOR_VERSION)" in \
>             6) echo ... ;; \
>             7) echo ... ;; \
>             *)  echo ... ;; \
>             esac)
>
> I haven't really try it yet.

What does either really buy us? It's really just bike shedding and
unrelated to fixing the problem at hand.

I have another idea which is to use llvm-config and avoid the
conditionals altogether. I haven't looked into that closely though.

Rob