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 |
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
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
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
>>>>> "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 --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 +
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