Fix i.MX93 compilation warnings related to mis-formatting / casting 64-bit types #8529

LaurentiuM1234 · 2023-11-27T11:46:07Z

Currently, building SOF on a 64-bit system (i.e: i.MX93) leads to compilation warnings such as:

"format '%d' expects argument of type 'int', but argument 5 has type 'long unsigned int'" or

"cast to pointer from integer of different size"

This is because pointers are 64-bit and size_t resolves to a 64-bit type. As such, casting a pointer to uint32_t when printing leads to int-to-pointer-cast warnings. Also, printing a size_t variable using the "%d" or "%u" formats leads to format warnings.

To fix this, cast all size_t variables to uint32_t when printing. Pointers shall also be casted to uintptr_t before casting to uint32_t. This is fine to do because i.MX93 uses 32-bit addresses anyways.

This in conjunction with #8528 fixes all compilation warnings for i.MX93.
This fix is not that great since it forces 64-bit types to 32-bit and counts on the fact that you won't have values that need 64-bit encoding, but it's a start I guess.

lgirdwood

Don't think we can do this yet as we are more optimised for size vs portability.

https://stackoverflow.com/questions/1403074/printf-with-sizeof-on-32-vs-64-platforms-how-do-i-handle-format-code-in-platfor

@LaurentiuM1234 @dbaluta btw, can get this platform as a build check in CI ?

LaurentiuM1234 · 2023-11-27T16:36:43Z

Don't think we can do this yet as we are more optimised for size vs portability.

Understood. Frankly, I think forcing 64-bit types to 32-bit is not really the way to go so if anyone thinks this is a bad idea or opposes to this being merged then I'm all for dropping this and dealing with it when it becomes necessary. It would have been nice to get rid of all of those annoying warnings as it's become very hard to find new warnings in the sea of already existing warnings.

@LaurentiuM1234 @dbaluta btw, can get this platform as a build check in CI ?

we already have a CI job building SOF for i.MX93. It's just that it's separate from the one building it for all other i.MX platforms.

LaurentiuM1234 · 2023-11-27T16:54:35Z

Marking this as a draft for now as I'm not sure about this solution

marc-hb · 2023-11-27T23:41:14Z

Anything wrong with %zd? I haven't actually run it but it seems to compile with both Zephyr and XTOS.

marc-hb · 2023-11-27T23:47:49Z

Anything wrong with %zd? I haven't actually run it but it seems to compile with both Zephyr and XTOS.

This probably requires skipping the z character in sof/tools/logger/convert.c#process_params(). The old sof-logger only supports 32bits anyway.

As long as you don't plan to actually use sof/tools/logger with aarch64 then it should be fine.

lyakh

let's use %zu please

src/audio/data_blob.c

LaurentiuM1234 · 2023-11-28T13:42:41Z

This probably requires skipping the z character in sof/tools/logger/convert.c#process_params(). The old sof-logger only supports 32bits anyway.

As long as you don't plan to actually use sof/tools/logger with aarch64 then it should be fine.

sof-logger won't be used on i.MX93 but it's currently being used on the other i.MX boards. I've tested the logger on i.MX8QM and seems like it works although this is probably because my test scenario doesn't reach the modified print statements. @dbaluta @iuliana-prodan what's your take on this? Do we proceed with this and risk breaking sof-logger (assuming this would even break it)? Theoretically we should be ready to switch to serial debugging via Zephyr on all boards (except ULP)(also, this isn't the case for XTOS). I'm assuming this won't break the internal tests as afaik sof-logger isn't used there.

I'm assuming Intel no longer uses sof-logger, right?

EDIT: this may impact MTK and AMD though but I'm not 100% sure what their setup is so I can't really tell.

iuliana-prodan · 2023-11-28T14:12:05Z

This probably requires skipping the z character in sof/tools/logger/convert.c#process_params(). The old sof-logger only supports 32bits anyway.
As long as you don't plan to actually use sof/tools/logger with aarch64 then it should be fine.

sof-logger won't be used on i.MX93 but it's currently being used on the other i.MX boards. I've tested the logger on i.MX8QM and seems like it works although this is probably because my test scenario doesn't reach the modified print statements. @dbaluta @iuliana-prodan what's your take on this? Do we proceed with this and risk breaking sof-logger (assuming this would even break it)? Theoretically we should be ready to switch to serial debugging via Zephyr on all boards (except ULP)(also, this isn't the case for XTOS). I'm assuming this won't break the internal tests as afaik sof-logger isn't used there.

I'm assuming Intel no longer uses sof-logger, right?

EDIT: this may impact MTK and AMD though but I'm not 100% sure what their setup is so I can't really tell.

You tested sof-logger, with these changes on 8QM and is ok, right?
imx93 is the closest to zephyr native drivers so we will use serial debugger there.
And, as @marc-hb pointed out, the sof-logger might break on arch64, so for the rest it should be fine.

Therefore, I'm approving this!

LaurentiuM1234 · 2023-11-28T15:08:09Z

You tested sof-logger, with these changes on 8QM and is ok, right?

Yep, seems like it works fine, but please keep in mind that I've only tested a couple of aplays and nothing more so I can't guarantee that this won't break sof-logger

And, as @marc-hb pointed out, the sof-logger might break on arch64, so for the rest it should be fine.

ACK.

If everyone's OK with this then I'm going to remove from draft. As for the pointer downcast to 32-bit: we should be OK for now as there's no 64-bit platform using addresses encoded in 64 bits.

marc-hb · 2023-11-28T16:46:24Z

I've tested the logger on i.MX8QM and seems like it works although this is probably because my test scenario doesn't reach the modified print statements

Can you modify one statement that is actually in use (something outside error handling) and print some magic number with %zu in that statement? If that one works and that number is logged properly then I think you can blindly change the other ones.

I'm assuming Intel no longer uses sof-logger, right?

Not in the main branch, no. Still in use in stable-v2.2.

marc-hb · 2023-11-28T16:48:11Z

src/audio/module_adapter/module_adapter.c

@@ -48,7 +48,7 @@ struct comp_dev *module_adapter_new(const struct comp_driver *drv,

 	if (!config) {
 		comp_cl_err(drv, "module_adapter_new(), wrong input params! drv = %x config = %x",
-			    (uint32_t)drv, (uint32_t)config);
+			    (uint32_t)(uintptr_t)drv, (uint32_t)(uintptr_t)config);


Double casts are very rarely needed so they need at least a short comment/justification. I see you mentioned something in the commit message but that's too far away from the code.

Also: why not use %zu / (size_t) here?

one could use PRIxPTR to print uintptr_t directly

Thanks for the suggestions! Decided to go with Marc's suggestion for now. Avoiding the PRI* macros for now because of some interesting points from zephyrproject-rtos/zephyr#46032.

marc-hb · 2023-11-28T16:51:19Z

src/audio/module_adapter/module/generic.c

-		comp_err(dev, "module_load_config(): wrong input params! dev %x, cfg %x size %d",
-			 (uint32_t)dev, (uint32_t)cfg, size);
+		comp_err(dev, "module_load_config(): wrong input params! dev %x, cfg %x size %zu",
+			 (uint32_t)(uintptr_t)dev, (uint32_t)(uintptr_t)cfg, size);


marc-hb · 2023-11-28T16:56:20Z

src/include/sof/drivers/mu.h

 }

 static inline void imx_mu_write(uint32_t val, uint32_t reg)
 {
-	*((volatile uint32_t*)(MU_BASE + reg)) = val;
+	*((volatile uint32_t*)((uintptr_t)(MU_BASE + reg))) = val;


Again, MU_BASE is a literal so it is really not intuitive why this is needed. Please add a short explanation in the code (and this one does not seem explained by the commit message)

Please do this instead (tested)

--- a/src/platform/imx93_a55/include/platform/lib/memory.h +++ b/src/platform/imx93_a55/include/platform/lib/memory.h @@ -49,7 +49,7 @@ #define HEAPMEM_SIZE 0x00010000 /* SOF uses A side of the WAKEUPMIX MU */ -#define MU_BASE 0x42430000 +#define MU_BASE ((uintptr_t)0x42430000) /* SOF uses EDMA2 (a.k.a EDMA4 in the TRM) */ #define EDMA2_BASE 0x42010000

Or char * if you prefer.

Fun fact: C++23 allows 0x42430000Z

Fixed this by adding the ULL suffix. This ought to make MU_BASE 64-bit.

btian1 · 2023-11-29T02:34:21Z

src/audio/data_blob.c

@@ -184,7 +184,7 @@ int comp_data_blob_set(struct comp_data_blob_handler *blob_handler,
 	}
 #endif

-	comp_dbg(blob_handler->dev, "comp_data_blob_set_cmd() pos = %d, fragment size = %d",
+	comp_dbg(blob_handler->dev, "comp_data_blob_set_cmd() pos = %d, fragment size = %zu",


how about lld?

zd is arch-dependent, lld is typically not.

marc-hb

I've tested the logger on i.MX8QM and seems like it works although this is probably because my test scenario doesn't reach the modified print statements

Can you modify one statement that is actually in use (something outside error handling) and print some magic number with %zu in that statement? If that one works and that number is logged properly then I think you can blindly change the other ones.

Please test %zu with the sof-logger "for real" in at least one place before merging this great-looking but massive search/replace.

marc-hb · 2023-11-29T05:53:44Z

src/include/sof/drivers/mu.h

 }

 static inline void imx_mu_write(uint32_t val, uint32_t reg)
 {
-	*((volatile uint32_t*)(MU_BASE + reg)) = val;
+	*((volatile uint32_t*)((uintptr_t)(MU_BASE + reg))) = val;


Please do this instead (tested)

--- a/src/platform/imx93_a55/include/platform/lib/memory.h +++ b/src/platform/imx93_a55/include/platform/lib/memory.h @@ -49,7 +49,7 @@ #define HEAPMEM_SIZE 0x00010000 /* SOF uses A side of the WAKEUPMIX MU */ -#define MU_BASE 0x42430000 +#define MU_BASE ((uintptr_t)0x42430000) /* SOF uses EDMA2 (a.k.a EDMA4 in the TRM) */ #define EDMA2_BASE 0x42010000

Or char * if you prefer.

Fun fact: C++23 allows 0x42430000Z

marc-hb · 2023-11-29T06:00:07Z

The only failure in https://sof-ci.01.org/sof-pr-viewer/#/build/PR8529/build13187149 (to which you don't have access, sorry about that) was a "west update" failure, so don't worry about it.

dbaluta · 2023-11-29T10:00:33Z

@LaurentiuM1234 Let's also fix this:

WARNING: line length of 101 exceeds 100 columns
#190: FILE: src/audio/mux/mux.c:218:
+		comp_err(dev, "mux_init(): blob size %zu exceeds %zu", cfg->size, MUX_BLOB_MAX_SIZE);

LaurentiuM1234 · 2023-11-29T10:29:55Z

Please test %zu with the sof-logger "for real" in at least once place before merging this great-looking but massive search/replace.

With the following change:

diff --git a/src/ipc/ipc3/handler.c b/src/ipc/ipc3/handler.c
index b7884fcda..b41038933 100644
--- a/src/ipc/ipc3/handler.c
+++ b/src/ipc/ipc3/handler.c
@@ -1637,6 +1637,8 @@ void ipc_cmd(struct ipc_cmd_hdr *_hdr)
                tr_info(&ipc_tr, "ipc: new cmd 0x%x", hdr->cmd);
        }
 
+       tr_err(&ipc_tr, "Hello, world with number %zu", 0xdeadbeef);
+
        type = iGS(hdr->cmd);
 
        switch (type) {

I get the following messages in the logger (trace, -t):

src/ipc/ipc3/handler.c:1640 ERROR Hello, world with number 3735928559

which seems fine.

Without trace (-t), the logger has the following output:

rerror: Invalid filename length 1852399458 or ldc file does not match firmware
error: read_entry_from_ldc_file(0x20013100) returned -22
error: fetch_entry() failed with: -22, aborting
Skipped 0 bytes after the last statement. Potential mailbox wrap, check the start of the output for later logs.

which doesn't seem to be related.

LaurentiuM1234 · 2023-11-29T11:54:33Z

@LaurentiuM1234 Let's also fix this:

WARNING: line length of 101 exceeds 100 columns
#190: FILE: src/audio/mux/mux.c:218:
+		comp_err(dev, "mux_init(): blob size %zu exceeds %zu", cfg->size, MUX_BLOB_MAX_SIZE);

should be fixed with cc4eb17. We could also squash this into the initial PR that causes this issue if you'd like.

dbaluta · 2023-11-29T11:57:37Z

should be fixed with cc4eb17. We could also squash this into the initial PR that causes this issue if you'd like.

I'm fine that no need to squash it.

tested

marc-hb · 2023-11-29T17:02:06Z

I'm fine that no need to squash it.

Checkpatch is only checking... submitted patches one by one. So submitting a separate fixup commit will not turn things green in this PR and it will not help other, unrelated PRs either (cause they're not affected)

A separate fixup commit will only help if/when someone changes the same line again. But then they're likely to break that line themselves anyway.

Currently, building SOF on a 64-bit platform (i.e: i.MX93) leads to compilation warnings such as: "format '%d' expects argument of type 'int', but argument 5 has type 'long unsigned int'" This is because size_t is 64-bit on 64-bit platforms. As such, to get rid of these compilation warnings, use the %zu specifier when printing a size_t variable. Signed-off-by: Laurentiu Mihalcea <[email protected]>

Currently, the imx_mu_write() and imx_mu_read() functions cast MU_BASE's value to a pointer. On 64-bit platforms such as i.MX93 this leads to compilation warnings because the value of MU_BASE is 32-bit while the pointers are 64-bit. As such, to solve this, add the ULL suffix to the definition of MU_BASE to make it 64-bit. Signed-off-by: Laurentiu Mihalcea <[email protected]>

On 64-bit platforms such as i.MX93 the pointers are 64-bit. As such, casting them to uint32_t leads to compilation warnings such as: "cast to pointer from integer of different size" To fix this, cast to size_t which, on 32-bit platforms is 32-bit and 64-bit on 64-bit platforms. Printing will be done via "%zx". Signed-off-by: Laurentiu Mihalcea <[email protected]>

LaurentiuM1234 · 2023-11-29T17:18:19Z

I'm fine that no need to squash it.

Checkpatch is only checking... submitted patches one by one. So submitting a separate fixup commit will not turn things green in this PR and it will not help other, unrelated PRs either.

A separate fixup commit will only help if/when someone changes the same line again. But then they're likely to break that line themselves anyway.

squashed it after all. The issue was introduced by b1fabd4 anyways.

keqiaozhang · 2023-11-30T06:27:34Z

SOFCI TEST

kv2019i · 2023-11-30T08:23:13Z

Ready to go, waiting for CI to complete, but based on earlier runs (and PR content), this should pass.

lyakh · 2023-11-30T09:27:21Z

src/audio/module_adapter/module/generic.c

-		comp_err(dev, "module_load_config(): wrong input params! dev %x, cfg %x size %zu",
-			 (uint32_t)dev, (uint32_t)cfg, size);
+		comp_err(dev, "module_load_config(): wrong input params! dev %zx, cfg %zx size %zu",
+			 (size_t)dev, (size_t)cfg, size);


why are we actually not just printing these as pointers per %p?

I remember some confusion about whether sof-logger supports %p (I think it does)

kv2019i · 2023-11-30T18:42:56Z

The actual build part of "sof-ci/jenkins/pr-build" seems have been successful, so I'll proceed to merge this.

marc-hb · 2023-11-30T20:11:38Z

Awesome, now that we have a warnings-free build, let's keep it that way. Please review very small -Werror PR:

[SKIP SOF-TEST] .github/zephyr: add -Werror to LP64 compilation #8559

LaurentiuM1234 changed the title ~~Cast size_t to uint32_t and pointers to uintptr_t before uint32_t~~ Fix i.MX93 compilation warnings related to mis-formatting / casting 64-bit types Nov 27, 2023

LaurentiuM1234 marked this pull request as ready for review November 27, 2023 12:00

LaurentiuM1234 requested review from ranj063, jxstelter, tlissows, singalsu, fkwasowi, iganakov, abonislawski, lgirdwood, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners November 27, 2023 12:00

dbaluta approved these changes Nov 27, 2023

View reviewed changes

lgirdwood approved these changes Nov 27, 2023

View reviewed changes

LaurentiuM1234 requested a review from marc-hb November 27, 2023 16:39

LaurentiuM1234 marked this pull request as draft November 27, 2023 16:54

lyakh reviewed Nov 28, 2023

View reviewed changes

src/audio/data_blob.c Outdated Show resolved Hide resolved

LaurentiuM1234 force-pushed the imx93_ci_build_fix branch from 5f01ab3 to be2729a Compare November 28, 2023 13:17

dbaluta approved these changes Nov 28, 2023

View reviewed changes

iuliana-prodan approved these changes Nov 28, 2023

View reviewed changes

LaurentiuM1234 marked this pull request as ready for review November 28, 2023 15:08

marc-hb reviewed Nov 28, 2023

View reviewed changes

btian1 reviewed Nov 29, 2023

View reviewed changes

marc-hb previously requested changes Nov 29, 2023

View reviewed changes

LaurentiuM1234 force-pushed the imx93_ci_build_fix branch from be2729a to cc4eb17 Compare November 29, 2023 11:49

marc-hb approved these changes Nov 29, 2023

View reviewed changes

LaurentiuM1234 added 3 commits November 29, 2023 19:16

LaurentiuM1234 force-pushed the imx93_ci_build_fix branch from cc4eb17 to 826892e Compare November 29, 2023 17:16

kv2019i approved these changes Nov 30, 2023

View reviewed changes

lyakh reviewed Nov 30, 2023

View reviewed changes

kv2019i merged commit a8ad6eb into thesofproject:main Nov 30, 2023
42 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix i.MX93 compilation warnings related to mis-formatting / casting 64-bit types #8529

Fix i.MX93 compilation warnings related to mis-formatting / casting 64-bit types #8529

LaurentiuM1234 commented Nov 27, 2023 •

edited

Loading

lgirdwood left a comment

LaurentiuM1234 commented Nov 27, 2023

LaurentiuM1234 commented Nov 27, 2023

marc-hb commented Nov 27, 2023

marc-hb commented Nov 27, 2023

lyakh left a comment

LaurentiuM1234 commented Nov 28, 2023 •

edited

Loading

iuliana-prodan commented Nov 28, 2023

LaurentiuM1234 commented Nov 28, 2023

marc-hb commented Nov 28, 2023

marc-hb Nov 28, 2023

lyakh Nov 29, 2023

LaurentiuM1234 Nov 29, 2023

marc-hb Nov 28, 2023

marc-hb Nov 28, 2023

marc-hb Nov 29, 2023 •

edited

Loading

LaurentiuM1234 Nov 29, 2023

btian1 Nov 29, 2023

marc-hb Nov 29, 2023

marc-hb left a comment •

edited

Loading

marc-hb Nov 29, 2023 •

edited

Loading

marc-hb commented Nov 29, 2023

dbaluta commented Nov 29, 2023

LaurentiuM1234 commented Nov 29, 2023

LaurentiuM1234 commented Nov 29, 2023

dbaluta commented Nov 29, 2023

marc-hb commented Nov 29, 2023 •

edited

Loading

LaurentiuM1234 commented Nov 29, 2023

keqiaozhang commented Nov 30, 2023

kv2019i commented Nov 30, 2023

lyakh Nov 30, 2023

marc-hb Nov 30, 2023

kv2019i commented Nov 30, 2023

marc-hb commented Nov 30, 2023

Fix i.MX93 compilation warnings related to mis-formatting / casting 64-bit types #8529

Fix i.MX93 compilation warnings related to mis-formatting / casting 64-bit types #8529

Conversation

LaurentiuM1234 commented Nov 27, 2023 • edited Loading

lgirdwood left a comment

Choose a reason for hiding this comment

LaurentiuM1234 commented Nov 27, 2023

LaurentiuM1234 commented Nov 27, 2023

marc-hb commented Nov 27, 2023

marc-hb commented Nov 27, 2023

lyakh left a comment

Choose a reason for hiding this comment

LaurentiuM1234 commented Nov 28, 2023 • edited Loading

iuliana-prodan commented Nov 28, 2023

LaurentiuM1234 commented Nov 28, 2023

marc-hb commented Nov 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb left a comment • edited Loading

Choose a reason for hiding this comment

marc-hb Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

marc-hb commented Nov 29, 2023

dbaluta commented Nov 29, 2023

LaurentiuM1234 commented Nov 29, 2023

LaurentiuM1234 commented Nov 29, 2023

dbaluta commented Nov 29, 2023

marc-hb commented Nov 29, 2023 • edited Loading

LaurentiuM1234 commented Nov 29, 2023

keqiaozhang commented Nov 30, 2023

kv2019i commented Nov 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kv2019i commented Nov 30, 2023

marc-hb commented Nov 30, 2023

LaurentiuM1234 commented Nov 27, 2023 •

edited

Loading

LaurentiuM1234 commented Nov 28, 2023 •

edited

Loading

marc-hb Nov 29, 2023 •

edited

Loading

marc-hb left a comment •

edited

Loading

marc-hb Nov 29, 2023 •

edited

Loading

marc-hb commented Nov 29, 2023 •

edited

Loading