Android: Fix LLVM duplicated symbols linking for N and M

Submitted by Rob Herring on Aug. 18, 2017, 7:46 p.m.

Details

Message ID 20170818194629.15890-1-robh@kernel.org
State New
Headers show
Series "Android: Fix LLVM duplicated symbols linking for N and M" ( rev: 1 ) in Mesa

Browsing this patch as part of:
"Android: Fix LLVM duplicated symbols linking for N and M" rev 1 in Mesa
<< prev patch [1/1] next patch >>

Commit Message

Rob Herring Aug. 18, 2017, 7:46 p.m.
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.

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.

Fixes: 26aee6f4d5a ("Android: rework LLVM build support")
Reported-by: Mauro Rossi <issor.oruam@gmail.com>
Cc: Emil Velikov <emil.l.velikov@gmail.com>
Cc: 17.2 <mesa-stable@lists.freedesktop.org>
Signed-off-by: Qiang Yu <Qiang.Yu@amd.com>
Signed-off-by: Rob Herring <robh@kernel.org>
---
 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(-)

Patch hide | download patch | download mbox

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),) \
   $(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)
 endef
 
 # add subdirectories
diff --git a/src/amd/Android.common.mk b/src/amd/Android.common.mk
index 7d08bfd31d79..4e2d0f9c2ffa 100644
--- a/src/amd/Android.common.mk
+++ b/src/amd/Android.common.mk
@@ -55,9 +55,7 @@  LOCAL_C_INCLUDES := \
 	$(call generated-sources-dir-for,STATIC_LIBRARIES,libmesa_nir,,)/nir \
 	$(MESA_TOP)/src/gallium/include \
 	$(MESA_TOP)/src/gallium/auxiliary \
-	$(intermediates)/common \
-	external/llvm/include \
-	external/llvm/device/include
+	$(intermediates)/common
 
 LOCAL_EXPORT_C_INCLUDE_DIRS := \
 	$(LOCAL_PATH)/common
diff --git a/src/gallium/drivers/radeon/Android.mk b/src/gallium/drivers/radeon/Android.mk
index eb1a32182bb0..c2d3a1cbce60 100644
--- a/src/gallium/drivers/radeon/Android.mk
+++ b/src/gallium/drivers/radeon/Android.mk
@@ -30,7 +30,7 @@  include $(CLEAR_VARS)
 
 LOCAL_SRC_FILES := $(C_SOURCES)
 
-LOCAL_SHARED_LIBRARIES := libdrm_radeon libLLVM
+LOCAL_SHARED_LIBRARIES := libdrm_radeon
 LOCAL_MODULE := libmesa_pipe_radeon
 
 ifeq ($(MESA_ENABLE_LLVM),true)
diff --git a/src/gallium/drivers/radeonsi/Android.mk b/src/gallium/drivers/radeonsi/Android.mk
index 65661a5ea7a5..e72b80c4e807 100644
--- a/src/gallium/drivers/radeonsi/Android.mk
+++ b/src/gallium/drivers/radeonsi/Android.mk
@@ -41,7 +41,7 @@  LOCAL_C_INCLUDES := \
 
 LOCAL_STATIC_LIBRARIES := libmesa_amd_common
 
-LOCAL_SHARED_LIBRARIES := libdrm_radeon libLLVM
+LOCAL_SHARED_LIBRARIES := libdrm_radeon
 LOCAL_MODULE := libmesa_pipe_radeonsi
 
 intermediates := $(call local-generated-sources-dir)

Comments

On 18 August 2017 at 20:46, Rob Herring <robh@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@gmail.com>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> Cc: 17.2 <mesa-stable@lists.freedesktop.org>
> Signed-off-by: Qiang Yu <Qiang.Yu@amd.com>
> Signed-off-by: Rob Herring <robh@kernel.org>

Reviewed-by: Emil Velikov <emil.velikov@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),) \
>    $(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)

-Emil
2017-08-19 8:27 GMT+08:00 Emil Velikov <emil.l.velikov@gmail.com>:
> On 18 August 2017 at 20:46, Rob Herring <robh@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@gmail.com>
>> Cc: Emil Velikov <emil.l.velikov@gmail.com>
>> Cc: 17.2 <mesa-stable@lists.freedesktop.org>
>> Signed-off-by: Qiang Yu <Qiang.Yu@amd.com>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>
> Reviewed-by: Emil Velikov <emil.velikov@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.

>>    $(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.
> gallium_dri_la_LDFLAGS += \
>       -Wl,--version-script=$(top_srcdir)/src/gallium/targets/dri/dri.sym
A nice trick to know about.

Regards,
Qiang
On Fri, Aug 18, 2017 at 8:53 PM, Chih-Wei Huang <cwhuang@android-x86.org> wrote:
> 2017-08-19 8:27 GMT+08:00 Emil Velikov <emil.l.velikov@gmail.com>:
>> On 18 August 2017 at 20:46, Rob Herring <robh@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@gmail.com>
>>> Cc: Emil Velikov <emil.l.velikov@gmail.com>
>>> Cc: 17.2 <mesa-stable@lists.freedesktop.org>
>>> Signed-off-by: Qiang Yu <Qiang.Yu@amd.com>
>>> Signed-off-by: Rob Herring <robh@kernel.org>
>>
>> Reviewed-by: Emil Velikov <emil.velikov@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
On 20 August 2017 at 20:57, Rob Herring <robh@kernel.org> wrote:

>
> What does either really buy us? It's really just bike shedding and
> unrelated to fixing the problem at hand.
>
Agreed - it's orthogonal to the issue at hand, I should have made this clearer.
It's an open question, which is not meant to hold the patch.

> I have another idea which is to use llvm-config and avoid the
> conditionals altogether. I haven't looked into that closely though.
>
That will be amazing, thank you.

-Emil
2017-08-18 21:46 GMT+02:00 Rob Herring <robh@kernel.org>:
> 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.
>
> 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.
>
> Fixes: 26aee6f4d5a ("Android: rework LLVM build support")
> Reported-by: Mauro Rossi <issor.oruam@gmail.com>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> Cc: 17.2 <mesa-stable@lists.freedesktop.org>
> Signed-off-by: Qiang Yu <Qiang.Yu@amd.com>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>  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),) \
>    $(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)
>  endef

Hi Rob,

I've just seen now that llvm include paths were removed compared to
the original version of this patch.
I had reported build errors without those on android-x86 development
mail thread.

I'm submitting a correction to mesa-dev ML, please review and apply it

It will be necessary also in 17.2 branch when this patch will be
backported to 17.2.0.
Thanks

Mauro

>
>  # add subdirectories
> diff --git a/src/amd/Android.common.mk b/src/amd/Android.common.mk
> index 7d08bfd31d79..4e2d0f9c2ffa 100644
> --- a/src/amd/Android.common.mk
> +++ b/src/amd/Android.common.mk
> @@ -55,9 +55,7 @@ LOCAL_C_INCLUDES := \
>         $(call generated-sources-dir-for,STATIC_LIBRARIES,libmesa_nir,,)/nir \
>         $(MESA_TOP)/src/gallium/include \
>         $(MESA_TOP)/src/gallium/auxiliary \
> -       $(intermediates)/common \
> -       external/llvm/include \
> -       external/llvm/device/include
> +       $(intermediates)/common
>
>  LOCAL_EXPORT_C_INCLUDE_DIRS := \
>         $(LOCAL_PATH)/common
> diff --git a/src/gallium/drivers/radeon/Android.mk b/src/gallium/drivers/radeon/Android.mk
> index eb1a32182bb0..c2d3a1cbce60 100644
> --- a/src/gallium/drivers/radeon/Android.mk
> +++ b/src/gallium/drivers/radeon/Android.mk
> @@ -30,7 +30,7 @@ include $(CLEAR_VARS)
>
>  LOCAL_SRC_FILES := $(C_SOURCES)
>
> -LOCAL_SHARED_LIBRARIES := libdrm_radeon libLLVM
> +LOCAL_SHARED_LIBRARIES := libdrm_radeon
>  LOCAL_MODULE := libmesa_pipe_radeon
>
>  ifeq ($(MESA_ENABLE_LLVM),true)
> diff --git a/src/gallium/drivers/radeonsi/Android.mk b/src/gallium/drivers/radeonsi/Android.mk
> index 65661a5ea7a5..e72b80c4e807 100644
> --- a/src/gallium/drivers/radeonsi/Android.mk
> +++ b/src/gallium/drivers/radeonsi/Android.mk
> @@ -41,7 +41,7 @@ LOCAL_C_INCLUDES := \
>
>  LOCAL_STATIC_LIBRARIES := libmesa_amd_common
>
> -LOCAL_SHARED_LIBRARIES := libdrm_radeon libLLVM
> +LOCAL_SHARED_LIBRARIES := libdrm_radeon
>  LOCAL_MODULE := libmesa_pipe_radeonsi
>
>  intermediates := $(call local-generated-sources-dir)
> --
> 2.11.0
>
On Mon, Aug 21, 2017 at 4:44 PM, Mauro Rossi <issor.oruam@gmail.com> wrote:
> 2017-08-18 21:46 GMT+02:00 Rob Herring <robh@kernel.org>:
>> 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.
>>
>> 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.
>>
>> Fixes: 26aee6f4d5a ("Android: rework LLVM build support")
>> Reported-by: Mauro Rossi <issor.oruam@gmail.com>
>> Cc: Emil Velikov <emil.l.velikov@gmail.com>
>> Cc: 17.2 <mesa-stable@lists.freedesktop.org>
>> Signed-off-by: Qiang Yu <Qiang.Yu@amd.com>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> ---
>>  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),) \
>>    $(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)
>>  endef
>
> Hi Rob,
>
> I've just seen now that llvm include paths were removed compared to
> the original version of this patch.

Which is why I posted the the changed version.

> I had reported build errors without those on android-x86 development
> mail thread.

I replied in that thread that there was a typo in the LLVM change. I
had device/include instead of include/device for the include path.
Were you able to test that?

> I'm submitting a correction to mesa-dev ML, please review and apply it
>
> It will be necessary also in 17.2 branch when this patch will be
> backported to 17.2.0.
> Thanks
>
> Mauro
>
>>
>>  # add subdirectories
>> diff --git a/src/amd/Android.common.mk b/src/amd/Android.common.mk
>> index 7d08bfd31d79..4e2d0f9c2ffa 100644
>> --- a/src/amd/Android.common.mk
>> +++ b/src/amd/Android.common.mk
>> @@ -55,9 +55,7 @@ LOCAL_C_INCLUDES := \
>>         $(call generated-sources-dir-for,STATIC_LIBRARIES,libmesa_nir,,)/nir \
>>         $(MESA_TOP)/src/gallium/include \
>>         $(MESA_TOP)/src/gallium/auxiliary \
>> -       $(intermediates)/common \
>> -       external/llvm/include \
>> -       external/llvm/device/include
>> +       $(intermediates)/common
>>
>>  LOCAL_EXPORT_C_INCLUDE_DIRS := \
>>         $(LOCAL_PATH)/common
>> diff --git a/src/gallium/drivers/radeon/Android.mk b/src/gallium/drivers/radeon/Android.mk
>> index eb1a32182bb0..c2d3a1cbce60 100644
>> --- a/src/gallium/drivers/radeon/Android.mk
>> +++ b/src/gallium/drivers/radeon/Android.mk
>> @@ -30,7 +30,7 @@ include $(CLEAR_VARS)
>>
>>  LOCAL_SRC_FILES := $(C_SOURCES)
>>
>> -LOCAL_SHARED_LIBRARIES := libdrm_radeon libLLVM
>> +LOCAL_SHARED_LIBRARIES := libdrm_radeon
>>  LOCAL_MODULE := libmesa_pipe_radeon
>>
>>  ifeq ($(MESA_ENABLE_LLVM),true)
>> diff --git a/src/gallium/drivers/radeonsi/Android.mk b/src/gallium/drivers/radeonsi/Android.mk
>> index 65661a5ea7a5..e72b80c4e807 100644
>> --- a/src/gallium/drivers/radeonsi/Android.mk
>> +++ b/src/gallium/drivers/radeonsi/Android.mk
>> @@ -41,7 +41,7 @@ LOCAL_C_INCLUDES := \
>>
>>  LOCAL_STATIC_LIBRARIES := libmesa_amd_common
>>
>> -LOCAL_SHARED_LIBRARIES := libdrm_radeon libLLVM
>> +LOCAL_SHARED_LIBRARIES := libdrm_radeon
>>  LOCAL_MODULE := libmesa_pipe_radeonsi
>>
>>  intermediates := $(call local-generated-sources-dir)
>> --
>> 2.11.0
>>
2017-08-21 23:57 GMT+02:00 Rob Herring <robh@kernel.org>:
> On Mon, Aug 21, 2017 at 4:44 PM, Mauro Rossi <issor.oruam@gmail.com> wrote:
>> 2017-08-18 21:46 GMT+02:00 Rob Herring <robh@kernel.org>:
>>> 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.
>>>
>>> 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.
>>>
>>> Fixes: 26aee6f4d5a ("Android: rework LLVM build support")
>>> Reported-by: Mauro Rossi <issor.oruam@gmail.com>
>>> Cc: Emil Velikov <emil.l.velikov@gmail.com>
>>> Cc: 17.2 <mesa-stable@lists.freedesktop.org>
>>> Signed-off-by: Qiang Yu <Qiang.Yu@amd.com>
>>> Signed-off-by: Rob Herring <robh@kernel.org>
>>> ---
>>>  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),) \
>>>    $(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)
>>>  endef
>>
>> Hi Rob,
>>
>> I've just seen now that llvm include paths were removed compared to
>> the original version of this patch.
>
> Which is why I posted the the changed version.
>
>> I had reported build errors without those on android-x86 development
>> mail thread.
>
> I replied in that thread that there was a typo in the LLVM change. I
> had device/include instead of include/device for the include path.
> Were you able to test that?

I had even read the message, but I was not completely "connected"
The current version is more than ok.

Sorry

Mauro

>
>> I'm submitting a correction to mesa-dev ML, please review and apply it
>>
>> It will be necessary also in 17.2 branch when this patch will be
>> backported to 17.2.0.
>> Thanks
>>
>> Mauro
>>
>>>
>>>  # add subdirectories
>>> diff --git a/src/amd/Android.common.mk b/src/amd/Android.common.mk
>>> index 7d08bfd31d79..4e2d0f9c2ffa 100644
>>> --- a/src/amd/Android.common.mk
>>> +++ b/src/amd/Android.common.mk
>>> @@ -55,9 +55,7 @@ LOCAL_C_INCLUDES := \
>>>         $(call generated-sources-dir-for,STATIC_LIBRARIES,libmesa_nir,,)/nir \
>>>         $(MESA_TOP)/src/gallium/include \
>>>         $(MESA_TOP)/src/gallium/auxiliary \
>>> -       $(intermediates)/common \
>>> -       external/llvm/include \
>>> -       external/llvm/device/include
>>> +       $(intermediates)/common
>>>
>>>  LOCAL_EXPORT_C_INCLUDE_DIRS := \
>>>         $(LOCAL_PATH)/common
>>> diff --git a/src/gallium/drivers/radeon/Android.mk b/src/gallium/drivers/radeon/Android.mk
>>> index eb1a32182bb0..c2d3a1cbce60 100644
>>> --- a/src/gallium/drivers/radeon/Android.mk
>>> +++ b/src/gallium/drivers/radeon/Android.mk
>>> @@ -30,7 +30,7 @@ include $(CLEAR_VARS)
>>>
>>>  LOCAL_SRC_FILES := $(C_SOURCES)
>>>
>>> -LOCAL_SHARED_LIBRARIES := libdrm_radeon libLLVM
>>> +LOCAL_SHARED_LIBRARIES := libdrm_radeon
>>>  LOCAL_MODULE := libmesa_pipe_radeon
>>>
>>>  ifeq ($(MESA_ENABLE_LLVM),true)
>>> diff --git a/src/gallium/drivers/radeonsi/Android.mk b/src/gallium/drivers/radeonsi/Android.mk
>>> index 65661a5ea7a5..e72b80c4e807 100644
>>> --- a/src/gallium/drivers/radeonsi/Android.mk
>>> +++ b/src/gallium/drivers/radeonsi/Android.mk
>>> @@ -41,7 +41,7 @@ LOCAL_C_INCLUDES := \
>>>
>>>  LOCAL_STATIC_LIBRARIES := libmesa_amd_common
>>>
>>> -LOCAL_SHARED_LIBRARIES := libdrm_radeon libLLVM
>>> +LOCAL_SHARED_LIBRARIES := libdrm_radeon
>>>  LOCAL_MODULE := libmesa_pipe_radeonsi
>>>
>>>  intermediates := $(call local-generated-sources-dir)
>>> --
>>> 2.11.0
>>>
On Sun, Aug 20, 2017 at 2:57 PM, Rob Herring <robh@kernel.org> wrote:
> On Fri, Aug 18, 2017 at 8:53 PM, Chih-Wei Huang <cwhuang@android-x86.org> wrote:
>> 2017-08-19 8:27 GMT+08:00 Emil Velikov <emil.l.velikov@gmail.com>:
>>> On 18 August 2017 at 20:46, Rob Herring <robh@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.

[...]

>>>>    $(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.

Well, the build is broken again because the version changed from O to
8 (and I'm not sure if master is going to change to P or 9 at some
point). So I went ahead and have this all coded up like this (I don't
see a simple way to build and run llvm-config):

  $(eval $(shell sed -n -e
's/.*\(LLVM_VERSION_MAJOR\).*\([0-9].*\)/\1:=\2/p'
external/llvm/device/include/llvm/Config/llvm-config.h)) \
  $(eval $(shell sed -n -e
's/.*\(LLVM_VERSION_MINOR\).*\([0-9].*\)/\1:=\2/p'
external/llvm/device/include/llvm/Config/llvm-config.h)) \
  $(eval LOCAL_CFLAGS +=
-DHAVE_LLVM=0x$(LLVM_VERSION_MAJOR)0$(LLVM_VERSION_MINOR))

Only one slight problem in that for master/O it reports 3.8 as the
version is 3.8.275480 which I think is the SVN version number. Not
sure what to do with that...

Rob
On 23 August 2017 at 17:50, Rob Herring <robh@kernel.org> wrote:
> On Sun, Aug 20, 2017 at 2:57 PM, Rob Herring <robh@kernel.org> wrote:
>> On Fri, Aug 18, 2017 at 8:53 PM, Chih-Wei Huang <cwhuang@android-x86.org> wrote:
>>> 2017-08-19 8:27 GMT+08:00 Emil Velikov <emil.l.velikov@gmail.com>:
>>>> On 18 August 2017 at 20:46, Rob Herring <robh@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.
>
> [...]
>
>>>>>    $(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.
>
> Well, the build is broken again because the version changed from O to
> 8 (and I'm not sure if master is going to change to P or 9 at some
> point). So I went ahead and have this all coded up like this (I don't
> see a simple way to build and run llvm-config):
>
Yay :-(

>   $(eval $(shell sed -n -e
> 's/.*\(LLVM_VERSION_MAJOR\).*\([0-9].*\)/\1:=\2/p'
> external/llvm/device/include/llvm/Config/llvm-config.h)) \
>   $(eval $(shell sed -n -e
> 's/.*\(LLVM_VERSION_MINOR\).*\([0-9].*\)/\1:=\2/p'
> external/llvm/device/include/llvm/Config/llvm-config.h)) \
>   $(eval LOCAL_CFLAGS +=
> -DHAVE_LLVM=0x$(LLVM_VERSION_MAJOR)0$(LLVM_VERSION_MINOR))
>
> Only one slight problem in that for master/O it reports 3.8 as the
> version is 3.8.275480 which I think is the SVN version number. Not
> sure what to do with that...
>
Indeed, seems like a SVN version. Not sure how much to care about the
PATCH version.
Leave it as 0 or use the SVN one - your call.

Ideally there will be an LLVM API to query that at runtime... but
that's topic for another day.

-Emil
On Wed, Aug 23, 2017 at 12:31 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 23 August 2017 at 17:50, Rob Herring <robh@kernel.org> wrote:
>> On Sun, Aug 20, 2017 at 2:57 PM, Rob Herring <robh@kernel.org> wrote:
>>> On Fri, Aug 18, 2017 at 8:53 PM, Chih-Wei Huang <cwhuang@android-x86.org> wrote:
>>>> 2017-08-19 8:27 GMT+08:00 Emil Velikov <emil.l.velikov@gmail.com>:
>>>>> On 18 August 2017 at 20:46, Rob Herring <robh@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.
>>
>> [...]
>>
>>>>>>    $(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.
>>
>> Well, the build is broken again because the version changed from O to
>> 8 (and I'm not sure if master is going to change to P or 9 at some
>> point). So I went ahead and have this all coded up like this (I don't
>> see a simple way to build and run llvm-config):
>>
> Yay :-(
>
>>   $(eval $(shell sed -n -e
>> 's/.*\(LLVM_VERSION_MAJOR\).*\([0-9].*\)/\1:=\2/p'
>> external/llvm/device/include/llvm/Config/llvm-config.h)) \
>>   $(eval $(shell sed -n -e
>> 's/.*\(LLVM_VERSION_MINOR\).*\([0-9].*\)/\1:=\2/p'
>> external/llvm/device/include/llvm/Config/llvm-config.h)) \
>>   $(eval LOCAL_CFLAGS +=
>> -DHAVE_LLVM=0x$(LLVM_VERSION_MAJOR)0$(LLVM_VERSION_MINOR))
>>
>> Only one slight problem in that for master/O it reports 3.8 as the
>> version is 3.8.275480 which I think is the SVN version number. Not
>> sure what to do with that...
>>
> Indeed, seems like a SVN version. Not sure how much to care about the
> PATCH version.
> Leave it as 0 or use the SVN one - your call.

Okay, I was a bit vague. The problem is the version is effectively 3.9
and the build breaks if we build with HAVE_LLVM=0x308 instead.

I don't think it would work on older versions either. N has 3.8.256229
and that is 3.8 (from mesa perspective). The M version was 3.6.svn,
but we pass 3.7 to mesa. So there's not really a programmatic way to
handle this.

Rob
Il 23/ago/2017 19:57, "Rob Herring" <robh@kernel.org> ha scritto:

On Wed, Aug 23, 2017 at 12:31 PM, Emil Velikov <emil.l.velikov@gmail.com>
wrote:
> On 23 August 2017 at 17:50, Rob Herring <robh@kernel.org> wrote:
>> On Sun, Aug 20, 2017 at 2:57 PM, Rob Herring <robh@kernel.org> wrote:
>>> On Fri, Aug 18, 2017 at 8:53 PM, Chih-Wei Huang <cwhuang@android-x86.org>
wrote:
>>>> 2017-08-19 8:27 GMT+08:00 Emil Velikov <emil.l.velikov@gmail.com>:
>>>>> On 18 August 2017 at 20:46, Rob Herring <robh@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.
>>
>> [...]
>>
>>>>>>    $(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.
>>
>> Well, the build is broken again because the version changed from O to
>> 8 (and I'm not sure if master is going to change to P or 9 at some
>> point). So I went ahead and have this all coded up like this (I don't
>> see a simple way to build and run llvm-config):
>>
> Yay :-(
>
>>   $(eval $(shell sed -n -e
>> 's/.*\(LLVM_VERSION_MAJOR\).*\([0-9].*\)/\1:=\2/p'
>> external/llvm/device/include/llvm/Config/llvm-config.h)) \
>>   $(eval $(shell sed -n -e
>> 's/.*\(LLVM_VERSION_MINOR\).*\([0-9].*\)/\1:=\2/p'
>> external/llvm/device/include/llvm/Config/llvm-config.h)) \
>>   $(eval LOCAL_CFLAGS +=
>> -DHAVE_LLVM=0x$(LLVM_VERSION_MAJOR)0$(LLVM_VERSION_MINOR))
>>
>> Only one slight problem in that for master/O it reports 3.8 as the
>> version is 3.8.275480 which I think is the SVN version number. Not
>> sure what to do with that...
>>
> Indeed, seems like a SVN version. Not sure how much to care about the
> PATCH version.
> Leave it as 0 or use the SVN one - your call.

Okay, I was a bit vague. The problem is the version is effectively 3.9
and the build breaks if we build with HAVE_LLVM=0x308 instead.

I don't think it would work on older versions either. N has 3.8.256229
and that is 3.8 (from mesa perspective). The M version was 3.6.svn,
but we pass 3.7 to mesa. So there's not really a programmatic way to
handle this.

Rob


LLVM version in Cmakelists.txt is indeed 3.9.0
Mauro