[RFC] trace: verify argument size by lyakh · Pull Request #3376 · thesofproject/sof

lyakh · 2020-09-02T12:29:17Z

It's useful for my debugging, and it's compile-time only, so we might merge it, but we don't have to, it's completely optional.

Currently SOF tracing only supports up to 32 bit arguments, using it with 64-bit arguments produces unpredictable results. Add compile-time checks to make sure all arguments are of correct size.

Oh, and yes, it currently breaks building, which is what it's supposed to do. So if we want to merge this we first have to fix those breakages (expect a patch soon).

paulstelian97 · 2020-09-02T13:04:12Z

Are the errors actual locations where you're using 64-bit variables improperly in traces?

lyakh · 2020-09-02T13:09:43Z

Are the errors actual locations where you're using 64-bit variables improperly in traces?

@paulstelian97 yes, you should be able to see that in CI logs

paulstelian97 · 2020-09-02T13:11:22Z

I assume you can add the fixes for those cases (in most cases I think a cast to (uint32_t) will suffice? ) in this same PR, or would it be a better idea to separate it?

lyakh · 2020-09-02T13:40:18Z

I assume you can add the fixes for those cases (in most cases I think a cast to (uint32_t) will suffice? ) in this same PR, or would it be a better idea to separate it?

@paulstelian97 exactly, let's see how many were not included in my test configuration

lyakh · 2020-09-02T15:16:59Z

Build failed for ARM (i.MX8*) in src/drivers/imx/interrupt.c::irq_handler() at

			tr_err(&irq_i_tr, "irq_handler(): IRQ storm, status "
			       PRIx64,
			       get_irqsteer_interrupts(line_index));

@dbaluta as far as I understand ARM doesn't have a magic bullet to make 64-bit tracing work, does it? So this has to be fixed too?

dbaluta · 2020-09-02T17:51:01Z

@dbaluta as far as I understand ARM doesn't have a magic bullet to make 64-bit tracing work, does it? So this has to be fixed too?

Yes. @paulstelian97 care to help when you have some time. I'm currently in vacation and getting back on September, 14.

paulstelian97 · 2020-09-02T17:53:38Z

Oh yeah, forgot about that one. I'll see it tomorrow, but basically I'd just fetch that into a 64-bit variable and print two hex parameters which are the 32-bit halves. It was broken (always showing zeros) before anyway.

lyakh · 2020-09-03T09:54:49Z

Oh yeah, forgot about that one. I'll see it tomorrow, but basically I'd just fetch that into a 64-bit variable and print two hex parameters which are the 32-bit halves. It was broken (always showing zeros) before anyway.

@dbaluta @paulstelian97 I've added a fix for that to the patch too

lgirdwood

This is good - it lets developers know which trace types are supported (so they dont read back garbage in the trace logs)
@mrajwa btw - did you see the KPB change - you could be reading some garbage in parts of your logs....

lgirdwood · 2020-09-03T10:46:42Z

trace_check_size_uint32_1()

lgirdwood · 2020-09-03T10:47:00Z

sizeof(uint32_t)

paulstelian97

What helps you to not get build errors when passing pointers to traces? I can't seem to identify the spot that fixes it...

SOF tracing can only handle up to 32-bit long values, attempts to trace 64-bit values produce unpredictable results. Fix such cases by making sure values are within range and casting them to 32 bits. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

paulstelian97

From my point of view this looks good. Do handle Liam's comments as I agree.

lyakh · 2020-09-04T06:57:50Z

SOFCI TEST

Currently SOF tracing only supports up to 32 bit arguments, using it with 64-bit arguments produces unpredictable results. Add compile- time checks to make sure all arguments are of correct size. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

The PP_NARG() macro can only handle integer type arguments, use META_COUNT_VARAGS_BEFORE_COMPILE() instead to avoid having to cast pointer types to integers. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

ktrzcinx · 2020-09-04T11:22:27Z

+#define trace_check_size_uint32_0() do { } while (0)
+
+#define trace_check_size_uint32_1(a) do {				\
+	STATIC_ASSERT(sizeof(a) <= sizeof(uint32_t),			\
+	trace_parameter_too_large);					\
+} while (0)
+
+#define trace_check_size_uint32_2(a, b) do {				\
+	  trace_check_size_uint32_1(a);					\
+	  trace_check_size_uint32_1(b);					\
+} while (0)
+
+#define trace_check_size_uint32_3(a, b, c) do {				\
+	  trace_check_size_uint32_1(a);					\
+	  trace_check_size_uint32_1(b);					\
+	  trace_check_size_uint32_1(c);					\
+} while (0)
+
+#define trace_check_size_uint32_4(a, b, c, d) do {			\
+	  trace_check_size_uint32_1(a);					\
+	  trace_check_size_uint32_1(b);					\
+	  trace_check_size_uint32_1(c);					\
+	  trace_check_size_uint32_1(d);					\
+} while (0)
+
+#define trace_check_args(...) META_CONCAT(trace_check_size_uint32_,	\
+	META_COUNT_VARAGS_BEFORE_COMPILE(__VA_ARGS__))(__VA_ARGS__)
+


@lyakh maybe we would like to use META_MAP(), then trace_check_size_uint32_2, trace_check_size_uint32_3, trace_check_size_uint32_4 could be removed and solution will be more flexible when possible number of max number of trace arguments change.

@ktrzcinx you mean like

#define trace_check_size_uint32_2(a, b) META_MAP(1, trace_check_size_uint32_1, a, b) #define trace_check_size_uint32_3(a, b, c) META_MAP(1, trace_check_size_uint32_1, a, b, c) #define trace_check_size_uint32_4(a, b, c, d) META_MAP(2, trace_check_size_uint32_2, a, b, c, d)

yeah, sure, why not make it a bit more confusing yet :-D Or just make the above a bit more compact by

#define trace_check_size_uint32_3(a, b, c) do { \ trace_check_size_uint32_2(a, b); \ trace_check_size_uint32_1(c); \ } while (0) #define trace_check_size_uint32_4(a, b, c, d) do { \ trace_check_size_uint32_2(a, b); \ trace_check_size_uint32_2(c, d); \ } while (0)

I think rather about something like:

#define trace_check_size_uint32_1(a) STATIC_ASSERT(sizeof(a) <= sizeof(uint32_t), trace_parameter_too_large); #define STATIC_CHECK_ARGS(...) do { META_RECURSE( \ META_MAP(1, META_DEFER(trace_check_size_uint32_1), __VA_ARGS__)); \ } while (0) #define _log_message(atomic, lvl, comp_class, ctx, id_1, id_2, \ format, ...) \ do { \ _DECLARE_LOG_ENTRY(lvl, format, comp_class, \ PP_NARG(__VA_ARGS__)); \ STATIC_CHECK_ARGS(__VA_ARGS__); \ STATIC_ASSERT( ...

*not tested

@ktrzcinx I tried that and it didn't compile. Maybe I'm using it wrongly, and I don't see any other uses of META_MAP() in the code, so, unless you volunteer to investigate, maybe it's better to stay with the current solution for now.

Given that we are close to v1.6 lets merge this today unless @ktrzcinx can update the macro as it's better than what we have today. We can always revisit after v1.6. I would also like us to revisit META_MAP() if there are no users.

It wasn't one line change, so I created separate PR covering this topic: #3393.

Great - @lyakh shall we close this one ?

@lgirdwood sure, would @ktrzcinx also pull the third patch from this series into his PR?

ktrzcinx · 2020-09-10T04:53:26Z

PR content moved to #3393

lyakh requested review from akloniex and xiulipan as code owners September 2, 2020 12:29

lyakh requested review from lgirdwood and ranj063 September 2, 2020 12:29

lyakh force-pushed the trace64 branch from 2a48a0c to 1b17eb3 Compare September 2, 2020 13:39

lyakh requested review from libinyang, mrajwa and tlauda as code owners September 2, 2020 13:39

lyakh force-pushed the trace64 branch from 1b17eb3 to 3066d90 Compare September 2, 2020 14:27

lyakh force-pushed the trace64 branch from 3066d90 to 2e59e78 Compare September 2, 2020 17:09

lyakh requested review from dbaluta, jajanusz, lbetlej, mmaka1 and plbossart as code owners September 2, 2020 17:09

lyakh force-pushed the trace64 branch from 2e59e78 to 783e1d8 Compare September 3, 2020 09:54

paulstelian97 reviewed Sep 3, 2020

View reviewed changes

Comment thread src/drivers/imx/interrupt.c Outdated

lgirdwood requested changes Sep 3, 2020

View reviewed changes

lyakh mentioned this pull request Sep 3, 2020

uuid: use a pointer instead of casting it to an integer #3383

Merged

paulstelian97 reviewed Sep 3, 2020

View reviewed changes

Comment thread src/audio/host.c Outdated

paulstelian97 reviewed Sep 3, 2020

View reviewed changes

paulstelian97 approved these changes Sep 3, 2020

View reviewed changes

lyakh force-pushed the trace64 branch from 783e1d8 to 25abea0 Compare September 3, 2020 13:56

lyakh force-pushed the trace64 branch from 25abea0 to b75cfbe Compare September 4, 2020 08:56

lyakh added 2 commits September 4, 2020 12:07

trace: use proper pointer formats

479ca01

The PP_NARG() macro can only handle integer type arguments, use META_COUNT_VARAGS_BEFORE_COMPILE() instead to avoid having to cast pointer types to integers. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh force-pushed the trace64 branch from b75cfbe to 479ca01 Compare September 4, 2020 10:08

ktrzcinx reviewed Sep 4, 2020

View reviewed changes

ktrzcinx mentioned this pull request Sep 7, 2020

trace: verify argument size #3393

Merged

ktrzcinx closed this Sep 10, 2020

lyakh deleted the trace64 branch October 12, 2020 12:14

Conversation

lyakh commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulstelian97 commented Sep 2, 2020

Uh oh!

lyakh commented Sep 2, 2020

Uh oh!

paulstelian97 commented Sep 2, 2020

Uh oh!

lyakh commented Sep 2, 2020

Uh oh!

lyakh commented Sep 2, 2020

Uh oh!

dbaluta commented Sep 2, 2020

Uh oh!

paulstelian97 commented Sep 2, 2020

Uh oh!

lyakh commented Sep 3, 2020

Uh oh!

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

paulstelian97 left a comment

Choose a reason for hiding this comment

Uh oh!

paulstelian97 left a comment

Choose a reason for hiding this comment

Uh oh!

lyakh commented Sep 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktrzcinx commented Sep 10, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lyakh commented Sep 2, 2020 •

edited

Loading