diff mbox series

[v5] package/optee-client: fix build warnings from 3.4.0

Message ID 20190325105251.5914-1-etienne.carriere@linaro.org
State Superseded
Headers show
Series [v5] package/optee-client: fix build warnings from 3.4.0 | expand

Commit Message

Etienne Carriere March 25, 2019, 10:52 a.m. UTC 
Add a patch over current optee-client 3.4.0 to fix build issues
reported by some toolchains with traces like:

  /path/to/build/optee-client-3.4.0/libteec/src/tee_client_api.c: In function 'TEEC_InitializeContext':
  /path/to/build/optee-client-3.4.0/libteec/src/tee_client_api.c:149:28: error: 'gen_caps' may be used uninitialized in this function [-Werror=maybe-uninitialized]
    ctx->reg_mem = gen_caps & TEE_GEN_CAP_REG_MEM;
                            ^
  /path/to/build/optee-client-3.4.0/libteec/src/tee_client_api.c: In function ‘TEEC_OpenSession’:
  /path/to/build/optee-client-3.4.0/libteec/src/tee_client_api.c:507:8: error: cast increases required alignment of target type [-Werror=cast-align]
    arg = (struct tee_ioctl_open_session_arg *)buf;
          ^
  /path/to/build/optee-client-3.4.0/libteec/src/tee_client_api.c: In function ‘TEEC_InvokeCommand’:
  /path/to/build/optee-client-3.4.0/libteec/src/tee_client_api.c:581:8: error: cast increases required alignment of target type [-Werror=cast-align]
    arg = (struct tee_ioctl_invoke_arg *)buf;

The optee-client patches have been in the OP-TEE project [1] & [2] and
will be available in the OP-TEE next release planned 3.5.0.

Fixes [3], [4], [5] and other failing builds reported by autobuild tests.

[1] https://github.com/OP-TEE/optee_client/pull/146
[2] https://github.com/OP-TEE/optee_client/pull/153
[3] http://autobuild.buildroot.net/results/80e78cb0bb955e912d6cbe5b30c9b024e7efc802
[4] http://autobuild.buildroot.net/results/a42c19897d03beb02fde2e7e6da25532be27d5ca
[5] http://autobuild.buildroot.net/results/827087f91b7481d1c3effd615172bbee86317962

Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
---
Changes v4 -> v5:
  - Fix buggy content sent in v3. Apologies.

Changes v3 -> v4:
  - Update patch from [2] outcome

Changes v2 -> v3:
  - Update patch from [1] outcome

Changes v1 -> v2:
  - Updated commit comment with traces of the build issues that are fixed.
---
 .../0001-libteec-fix-build-warnings.patch     | 132 ++++++++++++++++++
 1 file changed, 132 insertions(+)
 create mode 100644 package/optee-client/0001-libteec-fix-build-warnings.patch

Comments

Thomas Petazzoni March 25, 2019, 5:09 p.m. UTC | #1 
Hello Etienne,

On Mon, 25 Mar 2019 11:52:51 +0100
Etienne Carriere <etienne.carriere@linaro.org> wrote:

> Add a patch over current optee-client 3.4.0 to fix build issues
> reported by some toolchains with traces like:
> 
>   /path/to/build/optee-client-3.4.0/libteec/src/tee_client_api.c: In function 'TEEC_InitializeContext':
>   /path/to/build/optee-client-3.4.0/libteec/src/tee_client_api.c:149:28: error: 'gen_caps' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>     ctx->reg_mem = gen_caps & TEE_GEN_CAP_REG_MEM;
>                             ^
>   /path/to/build/optee-client-3.4.0/libteec/src/tee_client_api.c: In function ‘TEEC_OpenSession’:
>   /path/to/build/optee-client-3.4.0/libteec/src/tee_client_api.c:507:8: error: cast increases required alignment of target type [-Werror=cast-align]
>     arg = (struct tee_ioctl_open_session_arg *)buf;
>           ^
>   /path/to/build/optee-client-3.4.0/libteec/src/tee_client_api.c: In function ‘TEEC_InvokeCommand’:
>   /path/to/build/optee-client-3.4.0/libteec/src/tee_client_api.c:581:8: error: cast increases required alignment of target type [-Werror=cast-align]
>     arg = (struct tee_ioctl_invoke_arg *)buf;
> 
> The optee-client patches have been in the OP-TEE project [1] & [2] and
> will be available in the OP-TEE next release planned 3.5.0.
> 
> Fixes [3], [4], [5] and other failing builds reported by autobuild tests.
> 
> [1] https://github.com/OP-TEE/optee_client/pull/146
> [2] https://github.com/OP-TEE/optee_client/pull/153
> [3] http://autobuild.buildroot.net/results/80e78cb0bb955e912d6cbe5b30c9b024e7efc802
> [4] http://autobuild.buildroot.net/results/a42c19897d03beb02fde2e7e6da25532be27d5ca
> [5] http://autobuild.buildroot.net/results/827087f91b7481d1c3effd615172bbee86317962

Thanks for working on this. However, sorry for being picky, but there
are still a few things that could be done better.

First of all the pull request
https://github.com/OP-TEE/optee_client/pull/146 has been superseded by
https://github.com/OP-TEE/optee_client/pull/150, which has been merged
in commit
https://github.com/OP-TEE/optee_client/commit/9dbc61b3767ab1c3dfd0a19af02926b92ae09247

Your second pull request has been merged in commit
https://github.com/OP-TEE/optee_client/commit/16c8f548786c70df04d3a1e61bf89abce9b92389.

So referencing the pull requests is not really appropriate, referencing
the commits is much better.

Then, you squashed both changes in a single patch, this is not great.
We really prefer to have the upstream commits as-is, it makes things a
lot easier/clearer when bumping the package version.

Basically, to generate the patches, you should ideally do:

	git checkout v3.4.0
	git cherry-pick 9dbc61b3767ab1c3dfd0a19af02926b92ae09247
	git cherry-pick 16c8f548786c70df04d3a1e61bf89abce9b92389
	git format-patch -N HEAD~2

And use the two patches that have been generated.

However, I am a bit skeptical about the second commit: the commit log
says it fixes build issues with Clang, but Buildroot doesn't support
using Clang as a compiler, so why do we care ? Of course, the patch is
small, so it's not a big deal, but what is the reason for having this
patch ?

Thanks!

Thomas
Etienne Carriere March 25, 2019, 10:31 p.m. UTC | #2 
Hi Thomas,

On Mon, 25 Mar 2019 at 18:10, Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello Etienne,
>
>
> Thanks for working on this. However, sorry for being picky, but there
> are still a few things that could be done better.

Please do so.
And thanks for the feedback and details.

>
> First of all the pull request
> https://github.com/OP-TEE/optee_client/pull/146 has been superseded by
> https://github.com/OP-TEE/optee_client/pull/150, which has been merged
> in commit
> https://github.com/OP-TEE/optee_client/commit/9dbc61b3767ab1c3dfd0a19af02926b92ae09247
>
> Your second pull request has been merged in commit
> https://github.com/OP-TEE/optee_client/commit/16c8f548786c70df04d3a1e61bf89abce9b92389.
>
> So referencing the pull requests is not really appropriate, referencing
> the commits is much better.
>
> Then, you squashed both changes in a single patch, this is not great.
> We really prefer to have the upstream commits as-is, it makes things a
> lot easier/clearer when bumping the package version.
>
> Basically, to generate the patches, you should ideally do:
>
>         git checkout v3.4.0
>         git cherry-pick 9dbc61b3767ab1c3dfd0a19af02926b92ae09247
>         git cherry-pick 16c8f548786c70df04d3a1e61bf89abce9b92389
>         git format-patch -N HEAD~2
>
> And use the two patches that have been generated.

Ok, I see how you prefer patches to be handled.
I'll update.

>
> However, I am a bit skeptical about the second commit: the commit log
> says it fixes build issues with Clang, but Buildroot doesn't support
> using Clang as a compiler, so why do we care ? Of course, the patch is
> small, so it's not a big deal, but what is the reason for having this
> patch ?

The 2nd commit make the first fix stronger. Maybe gcc will complain
for this in a near future.
This change fixes an issue that is found in recent GCCs that is not
reported by not so old GCCs.
I should have even made the initial fix integrating the 2nd change.

etienne

>
> Thanks!
>
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Thomas Petazzoni March 26, 2019, 7:01 a.m. UTC | #3 
Hello,

On Mon, 25 Mar 2019 23:31:55 +0100
Etienne Carriere <etienne.carriere@linaro.org> wrote:

> > Thanks for working on this. However, sorry for being picky, but there
> > are still a few things that could be done better.  
> 
> Please do so.
> And thanks for the feedback and details.

And thanks for your understanding and perseverance!

> The 2nd commit make the first fix stronger. Maybe gcc will complain
> for this in a near future.
> This change fixes an issue that is found in recent GCCs that is not
> reported by not so old GCCs.
> I should have even made the initial fix integrating the 2nd change.

OK, thanks for the explanation. Then please include both patches but as
separate patches, as I explained in my previous reply.

Thanks!

Thomas
Peter Korsgaard March 27, 2019, 7:48 a.m. UTC | #4 
>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@bootlin.com> writes:

Hi,

 > Basically, to generate the patches, you should ideally do:

 > 	git checkout v3.4.0
 > 	git cherry-pick 9dbc61b3767ab1c3dfd0a19af02926b92ae09247
 > 	git cherry-pick 16c8f548786c70df04d3a1e61bf89abce9b92389
 > 	git format-patch -N HEAD~2

Please use git cherry-pick -xs to add a reference to the original commit
sha1 and add your signed-off-by.
diff mbox series

Patch

diff --git a/package/optee-client/0001-libteec-fix-build-warnings.patch b/package/optee-client/0001-libteec-fix-build-warnings.patch
new file mode 100644
index 0000000000..be87e9f46e
--- /dev/null
+++ b/package/optee-client/0001-libteec-fix-build-warnings.patch
@@ -0,0 +1,132 @@ 
+From dfb382a43c237f411d8994721cee5f9c216acb0b Mon Sep 17 00:00:00 2001
+From: Etienne Carriere <etienne.carriere@linaro.org>
+Date: Wed, 20 Mar 2019 10:01:23 +0100
+Subject: [PATCH] libteec: fix build warnings
+MIME-Version: 1.0
+Content-Type: text/plain; charset=UTF-8
+Content-Transfer-Encoding: 8bit
+
+Fix build warnings reported by the Buildroot team [1]:
+
+/home/thomas/projets/outputs/armv5-ctng-linux-gnueabi/build/optee-client-3.4.0/libteec/src/tee_client_api.c: In function 'TEEC_InitializeContext':
+/home/thomas/projets/outputs/armv5-ctng-linux-gnueabi/build/optee-client-3.4.0/libteec/src/tee_client_api.c:149:28: error: 'gen_caps' may be used uninitialized in this function [-Werror=maybe-uninitialized]
+    ctx->reg_mem = gen_caps & TEE_GEN_CAP_REG_MEM;
+                            ^
+/home/thomas/projets/buildroot/output/build/optee-client-3.4.0/libteec/src/tee_client_api.c: In function ‘TEEC_OpenSession’:
+/home/thomas/projets/buildroot/output/build/optee-client-3.4.0/libteec/src/tee_client_api.c:507:8: error: cast increases required alignment of target type [-Werror=cast-align]
+  arg = (struct tee_ioctl_open_session_arg *)buf;
+        ^
+/home/thomas/projets/buildroot/output/build/optee-client-3.4.0/libteec/src/tee_client_api.c: In function ‘TEEC_InvokeCommand’:
+/home/thomas/projets/buildroot/output/build/optee-client-3.4.0/libteec/src/tee_client_api.c:581:8: error: cast increases required alignment of target type [-Werror=cast-align]
+  arg = (struct tee_ioctl_invoke_arg *)buf;
+        ^
+
+This change is a squash of changes from [2] and [3].
+
+[1] http://lists.busybox.net/pipermail/buildroot/2019-February/243437.html
+[2] https://github.com/OP-TEE/optee_client/pull/146
+[3] https://github.com/OP-TEE/optee_client/pull/153
+
+Reported-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
+Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
+Signed-off-by: Victor Chong <victor.chong@linaro.org>
+---
+ libteec/src/tee_client_api.c | 36 +++++++++++++++++++++++-------------
+ 1 file changed, 23 insertions(+), 13 deletions(-)
+
+diff --git a/libteec/src/tee_client_api.c b/libteec/src/tee_client_api.c
+index 698092b..4d7b134 100644
+--- a/libteec/src/tee_client_api.c
++++ b/libteec/src/tee_client_api.c
+@@ -140,7 +140,7 @@ TEEC_Result TEEC_InitializeContext(const char *name, TEEC_Context *ctx)
+ 		return TEEC_ERROR_BAD_PARAMETERS;
+ 
+ 	for (n = 0; n < TEEC_MAX_DEV_SEQ; n++) {
+-		uint32_t gen_caps;
++		uint32_t gen_caps = 0;
+ 
+ 		snprintf(devname, sizeof(devname), "/dev/tee%zu", n);
+ 		fd = teec_open_dev(devname, name, &gen_caps);
+@@ -481,10 +481,13 @@ TEEC_Result TEEC_OpenSession(TEEC_Context *ctx, TEEC_Session *session,
+ 			uint32_t connection_method, const void *connection_data,
+ 			TEEC_Operation *operation, uint32_t *ret_origin)
+ {
+-	uint64_t buf[(sizeof(struct tee_ioctl_open_session_arg) +
+-			TEEC_CONFIG_PAYLOAD_REF_COUNT *
+-				sizeof(struct tee_ioctl_param)) /
+-			sizeof(uint64_t)] = { 0 };
++	const size_t arg_size = sizeof(struct tee_ioctl_open_session_arg) +
++				TEEC_CONFIG_PAYLOAD_REF_COUNT *
++					sizeof(struct tee_ioctl_param);
++	union {
++		struct tee_ioctl_open_session_arg arg;
++		uint8_t data[arg_size];
++	} buf;
+ 	struct tee_ioctl_buf_data buf_data;
+ 	struct tee_ioctl_open_session_arg *arg;
+ 	struct tee_ioctl_param *params;
+@@ -493,6 +496,8 @@ TEEC_Result TEEC_OpenSession(TEEC_Context *ctx, TEEC_Session *session,
+ 	TEEC_SharedMemory shm[TEEC_CONFIG_PAYLOAD_REF_COUNT];
+ 	int rc;
+ 
++	memset(&buf, 0, sizeof(buf));
++
+ 	(void)&connection_data;
+ 
+ 	if (!ctx || !session) {
+@@ -501,10 +506,10 @@ TEEC_Result TEEC_OpenSession(TEEC_Context *ctx, TEEC_Session *session,
+ 		goto out;
+ 	}
+ 
+-	buf_data.buf_ptr = (uintptr_t)buf;
++	buf_data.buf_ptr = (uintptr_t)&buf;
+ 	buf_data.buf_len = sizeof(buf);
+ 
+-	arg = (struct tee_ioctl_open_session_arg *)buf;
++	arg = &buf.arg;
+ 	arg->num_params = TEEC_CONFIG_PAYLOAD_REF_COUNT;
+ 	params = (struct tee_ioctl_param *)(arg + 1);
+ 
+@@ -555,10 +560,13 @@ void TEEC_CloseSession(TEEC_Session *session)
+ TEEC_Result TEEC_InvokeCommand(TEEC_Session *session, uint32_t cmd_id,
+ 			TEEC_Operation *operation, uint32_t *error_origin)
+ {
+-	uint64_t buf[(sizeof(struct tee_ioctl_invoke_arg) +
+-			TEEC_CONFIG_PAYLOAD_REF_COUNT *
+-				sizeof(struct tee_ioctl_param)) /
+-			sizeof(uint64_t)] = { 0 };
++	const size_t arg_size = sizeof(struct tee_ioctl_invoke_arg) +
++				TEEC_CONFIG_PAYLOAD_REF_COUNT *
++					sizeof(struct tee_ioctl_param);
++	union {
++		struct tee_ioctl_invoke_arg arg;
++		uint8_t data[arg_size];
++	} buf;
+ 	struct tee_ioctl_buf_data buf_data;
+ 	struct tee_ioctl_invoke_arg *arg;
+ 	struct tee_ioctl_param *params;
+@@ -567,6 +575,8 @@ TEEC_Result TEEC_InvokeCommand(TEEC_Session *session, uint32_t cmd_id,
+ 	TEEC_SharedMemory shm[TEEC_CONFIG_PAYLOAD_REF_COUNT];
+ 	int rc;
+ 
++	memset(&buf, 0, sizeof(buf));
++
+ 	if (!session) {
+ 		eorig = TEEC_ORIGIN_API;
+ 		res = TEEC_ERROR_BAD_PARAMETERS;
+@@ -575,10 +585,10 @@ TEEC_Result TEEC_InvokeCommand(TEEC_Session *session, uint32_t cmd_id,
+ 
+ 	bm_timestamp();
+ 
+-	buf_data.buf_ptr = (uintptr_t)buf;
++	buf_data.buf_ptr = (uintptr_t)&buf;
+ 	buf_data.buf_len = sizeof(buf);
+ 
+-	arg = (struct tee_ioctl_invoke_arg *)buf;
++	arg = &buf.arg;
+ 	arg->num_params = TEEC_CONFIG_PAYLOAD_REF_COUNT;
+ 	params = (struct tee_ioctl_param *)(arg + 1);
+ 
+-- 
+2.17.1
+