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

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

2017-08-23 Thread Mauro Rossi

Il 23/ago/2017 19:57, "Rob Herring"  ha scritto:

On Wed, Aug 23, 2017 at 12:31 PM, Emil Velikov 
wrote:
> On 23 August 2017 at 17:50, Rob Herring  wrote:
>> On Sun, Aug 20, 2017 at 2:57 PM, Rob Herring  wrote:
>>> On Fri, Aug 18, 2017 at 8:53 PM, Chih-Wei Huang 
wrote:
 2017-08-19 8:27 GMT+08:00 Emil Velikov :
> On 18 August 2017 at 20:46, Rob Herring  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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2017-08-23 Thread Rob Herring

On Wed, Aug 23, 2017 at 12:31 PM, Emil Velikov  wrote:
> On 23 August 2017 at 17:50, Rob Herring  wrote:
>> On Sun, Aug 20, 2017 at 2:57 PM, Rob Herring  wrote:
>>> On Fri, Aug 18, 2017 at 8:53 PM, Chih-Wei Huang  
>>> wrote:
 2017-08-19 8:27 GMT+08:00 Emil Velikov :
> On 18 August 2017 at 20:46, Rob Herring  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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2017-08-23 Thread Emil Velikov

On 23 August 2017 at 17:50, Rob Herring  wrote:
> On Sun, Aug 20, 2017 at 2:57 PM, Rob Herring  wrote:
>> On Fri, Aug 18, 2017 at 8:53 PM, Chih-Wei Huang  
>> wrote:
>>> 2017-08-19 8:27 GMT+08:00 Emil Velikov :
 On 18 August 2017 at 20:46, Rob Herring  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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2017-08-23 Thread Rob Herring

On Sun, Aug 20, 2017 at 2:57 PM, Rob Herring  wrote:
> On Fri, Aug 18, 2017 at 8:53 PM, Chih-Wei Huang  
> wrote:
>> 2017-08-19 8:27 GMT+08:00 Emil Velikov :
>>> On 18 August 2017 at 20:46, Rob Herring  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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2017-08-21 Thread Mauro Rossi

2017-08-21 23:57 GMT+02:00 Rob Herring :
> On Mon, Aug 21, 2017 at 4:44 PM, Mauro Rossi  wrote:
>> 2017-08-18 21:46 GMT+02:00 Rob Herring :
>>> 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 
>>> Cc: Emil Velikov 
>>> Cc: 17.2 
>>> Signed-off-by: Qiang Yu 
>>> Signed-off-by: Rob Herring 
>>> ---
>>>  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
>>> +++

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

2017-08-21 Thread Rob Herring

On Mon, Aug 21, 2017 at 4:44 PM, Mauro Rossi  wrote:
> 2017-08-18 21:46 GMT+02:00 Rob Herring :
>> 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 
>> Cc: Emil Velikov 
>> Cc: 17.2 
>> Signed-off-by: Qiang Yu 
>> Signed-off-by: Rob Herring 
>> ---
>>  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

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

2017-08-21 Thread Mauro Rossi

2017-08-18 21:46 GMT+02:00 Rob Herring :
> 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 
> Cc: Emil Velikov 
> Cc: 17.2 
> Signed-off-by: Qiang Yu 
> Signed-off-by: Rob Herring 
> ---
>  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
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2017-08-21 Thread Rob Herring

On Mon, Aug 21, 2017 at 12:47 PM, Emil Velikov  wrote:
> On 21 August 2017 at 17:17, Rob Herring  wrote:
>> On Fri, Aug 18, 2017 at 7:27 PM, Emil Velikov  
>> wrote:
>>> On 18 August 2017 at 20:46, Rob Herring  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
>>
>> About 60K smaller out of 12MB. Is that inline with what you'd expect?
>>
> Sounds a bit smaller than expected, but every little helps ;-)
>
>> before:
>>textdata bss dec hex filename
>> 12164054 420384  352476 12936914 c566d2
>> out/target/product/linaro_x86_64/system/lib64/dri/gallium_dri.so
>>
>> after:
>>textdata bss dec hex filename
>> 12104616 419816  352476 12876908 c47c6c
>> out/target/product/linaro_x86_64/system/lib64/dri/gallium_dri.so
>>
>>
>> The change is trivial, though I'm not sure if there's any downside to
>> setting --undefined-version (The Android build system turns this off).
>>
> There's no actual symbol versioning, just restriction of the actual
> exported symbols.
> Thus things should work w/o -Wl,--undefined-version so I wouldn't touch it.

No it doesn't, because it can't find __driDriverExtensions.

Rob
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2017-08-21 Thread Emil Velikov

On 21 August 2017 at 17:17, Rob Herring  wrote:
> On Fri, Aug 18, 2017 at 7:27 PM, Emil Velikov  
> wrote:
>> On 18 August 2017 at 20:46, Rob Herring  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
>
> About 60K smaller out of 12MB. Is that inline with what you'd expect?
>
Sounds a bit smaller than expected, but every little helps ;-)

> before:
>textdata bss dec hex filename
> 12164054 420384  352476 12936914 c566d2
> out/target/product/linaro_x86_64/system/lib64/dri/gallium_dri.so
>
> after:
>textdata bss dec hex filename
> 12104616 419816  352476 12876908 c47c6c
> out/target/product/linaro_x86_64/system/lib64/dri/gallium_dri.so
>
>
> The change is trivial, though I'm not sure if there's any downside to
> setting --undefined-version (The Android build system turns this off).
>
There's no actual symbol versioning, just restriction of the actual
exported symbols.
Thus things should work w/o -Wl,--undefined-version so I wouldn't touch it.

-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2017-08-21 Thread Rob Herring

On Fri, Aug 18, 2017 at 7:27 PM, Emil Velikov  wrote:
> On 18 August 2017 at 20:46, Rob Herring  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

About 60K smaller out of 12MB. Is that inline with what you'd expect?

before:
   textdata bss dec hex filename
12164054 420384  352476 12936914 c566d2
out/target/product/linaro_x86_64/system/lib64/dri/gallium_dri.so

after:
   textdata bss dec hex filename
12104616 419816  352476 12876908 c47c6c
out/target/product/linaro_x86_64/system/lib64/dri/gallium_dri.so


The change is trivial, though I'm not sure if there's any downside to
setting --undefined-version (The Android build system turns this off).

diff --git a/src/gallium/targets/dri/Android.mk
b/src/gallium/targets/dri/Android.mk
index 150b2e368e51..440dded0cebe 100644
--- a/src/gallium/targets/dri/Android.mk
+++ b/src/gallium/targets/dri/Android.mk
@@ -32,6 +32,8 @@ LOCAL_SRC_FILES := target.c

 LOCAL_CFLAGS :=

+LOCAL_LDFLAGS := -Wl,--version-script=$(LOCAL_PATH)/dri.sym
-Wl,--undefined-version
+
 LOCAL_SHARED_LIBRARIES := \
libdl \
liblog \
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2017-08-21 Thread Emil Velikov

On 20 August 2017 at 20:57, Rob Herring  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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2017-08-20 Thread Rob Herring

On Fri, Aug 18, 2017 at 8:53 PM, Chih-Wei Huang  wrote:
> 2017-08-19 8:27 GMT+08:00 Emil Velikov :
>> On 18 August 2017 at 20:46, Rob Herring  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 
>>> Cc: Emil Velikov 
>>> Cc: 17.2 
>>> Signed-off-by: Qiang Yu 
>>> Signed-off-by: Rob Herring 
>>
>> Reviewed-by: Emil Velikov 
>>
>>> ---
>>>  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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2017-08-18 Thread Yu, Qiang


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

Regards,
Qiang

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2017-08-18 Thread Chih-Wei Huang

2017-08-19 8:27 GMT+08:00 Emil Velikov :
> On 18 August 2017 at 20:46, Rob Herring  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 
>> Cc: Emil Velikov 
>> Cc: 17.2 
>> Signed-off-by: Qiang Yu 
>> Signed-off-by: Rob Herring 
>
> Reviewed-by: Emil Velikov 
>
>> ---
>>  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.

-- 
Chih-Wei
Android-x86 project
http://www.android-x86.org
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2017-08-18 Thread Emil Velikov

On 18 August 2017 at 20:46, Rob Herring  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 
> Cc: Emil Velikov 
> Cc: 17.2 
> Signed-off-by: Qiang Yu 
> Signed-off-by: Rob Herring 

Reviewed-by: Emil Velikov 

> ---
>  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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2017-08-18 Thread Rob Herring

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 
Cc: Emil Velikov 
Cc: 17.2 
Signed-off-by: Qiang Yu 
Signed-off-by: Rob Herring 
---
 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
 
 # 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

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

16 matches

Site Navigation

Mail list logo

Footer information