Fix i386 compilation #2396

plbossart · 2020-08-27T18:13:16Z

Two recent patches (not yet upstream) broke 32-bit compilation.

@RDharageswari and @lyakh FYI.

plbossart · 2020-08-27T18:54:48Z

SOFCI TEST

plbossart · 2020-08-27T20:21:45Z

wow. This is really bad, we have 3 new error cases with two test runs.

https://sof-ci.01.org/linuxpr/PR2396/build4392/devicetest/
https://sof-ci.01.org/linuxpr/PR2396/build4394/devicetest/

@fredoh9 @marc-hb I am afraid we claimed victory too early, the problems with CI targets continue. Gah.

marc-hb · 2020-08-28T01:15:03Z

I don't know alsabat. Any clue what the following means? Where can the 989.18 come from?
https://sof-ci.01.org/linuxpr/PR2396/build4392/devicetest/

2020-08-27 18:29:35 UTC [REMOTE_COMMAND] alsabat -Pplughw:0,0 --standalone -n 240000 -F 997
2020-08-27 18:29:36 UTC [REMOTE_COMMAND] alsabat -Cplughw:0,0 -F 997
 FAIL: Peak freq too low 989.18 Hz
alsa-utils version 1.2.2

Entering capture thread (ALSA).
Get period size: 3748  buffer size: 14994
Recording ...
Capture completed.

BAT analysis: signal has 65536 frames at 44100 Hz, 1 channels, 2 bytes per sample.

Channel 1 - Checking for target frequency 997.00 Hz
Amplitude: 17947.7; Percentage: [54]
Detected peak at 989.18 Hz of 21.09 dB
 Total 21.1 dB from 989.18 to 989.18 Hz
Detected peak at 997.26 Hz of 37.62 dB
 Total 42.2 dB from 991.20 to 1003.31 Hz
 PASS: Peak detected at target frequency
Detected at least 2 signal(s) in total

Return value is -1003

lyakh · 2020-08-28T06:07:10Z

sound/soc/sof/control.c

mmh, type-casts are evil(TM). Is there any specific reason for this one? It's only to fix the printk() format, right? The calculation, the value would be exactly the same if we just used %d? Maybe just do that then... Or is this because size_t is a different type on 32-bit builds, ARM?

be->max is an int, but after arithmetic with sizeof(), it's a "unsigned int" right. so would "%u" as printf syntax solve ths issue without a cast?

@lyakh yes size_t is different.
I tried earlier with a %d but it wouldn't work for x86_64. the %lu fails on 32-bit.
So I settled on a typecast to force the use of size_t, and let the compiler adjust.
If you have a better solution I am all ears, just trying to avoid an error reported upstream.

be->max is an int, but after arithmetic with sizeof(), it's a "unsigned int" right. so would "%u" as printf syntax solve ths issue without a cast?

@kv2019i no, sizeof() returns a size_t, which can be 32 or 64 bits, so that impacts the results.

The cast seems unnecessary, %zd should be enough. Have you tried with %zd only and no cast?

I think we can safely assume size_t has a rank always greater or equal to int.

In user space the following code compiles with both -m32 and -m64 without any warning:

int i = -1; size_t s = 42; printf ("%zd", i - s);

https://wiki.sei.cmu.edu/confluence/display/c/INT02-C.+Understand+integer+conversion+rules

Usual arithmetic conversions
3. If the operand that has unsigned integer type has rank greater than or equal to the rank of the type of the other operand, the operand with signed integer type is converted to the type of the operand with unsigned integer type.

That's because you switch to %lu (by accident?). I wrote no cast and %zu.

see e1cc344, didn't work.

~~I see that e1cc344 has %lu and not the %zu that I suggested.~~

it's %zu alright e1cc344#diff-6782c454986b1c6b9d4916315dd9dec7R391

My bad, I was misled by the error message in topology.c which you hadn't changed yet.

kv2019i

Ouch, good to have the i386 build in travis. See comments inline...

kv2019i · 2020-08-28T11:53:39Z

sound/soc/sof/control.c

be->max is an int, but after arithmetic with sizeof(), it's a "unsigned int" right. so would "%u" as printf syntax solve ths issue without a cast?

fix compilation warning in i386 mode. sound/soc/sof/control.c: In function ‘snd_sof_bytes_ext_volatile_get’: sound/soc/sof/control.c:388:35: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘unsigned int’ [-Wformat=] 388 | dev_err_ratelimited(scomp->dev, "error: user data size %d exceeds max size %lu.\n", | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The mix of integer and size_t generates different results in 32- and 64 bit mode, cast to size_t and use %zu format. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart · 2020-08-28T22:35:15Z

@lyakh if you have a better solution without the evil casts that's fine, please provide it as a follow-up

marc-hb

No cast + %zu does work with both i386 and x86_64, I just tested it locally at last.

However it requires not just one line changes but 5: four in control.c and one in topology.c

I search travis and found the two builds where you tried %zu and no cast. Github inconveniently hides them, maybe because it doesn't support force pushes:

https://travis-ci.org/github/thesofproject/linux/builds/722150716
https://travis-ci.org/github/thesofproject/linux/builds/722132973

None of these failed to compile no cast + %zu because.. they failed earlier, on topology.c!!

UPDATE: and now I just see you merged the spurious cast in 5 places... too bad.

marc-hb · 2020-08-28T23:43:57Z

sound/soc/sof/control.c

My bad, I was misled by the error message in topology.c which you hadn't changed yet.

marc-hb · 2020-08-31T04:22:55Z

Linux will apparently stop supporting 32bits soon https://lwn.net/Articles/829733/ :-D

paulstelian97 · 2020-08-31T07:34:04Z

Linux will apparently stop supporting 32bits soon https://lwn.net/Articles/829733/ :-D

Not making a subscription yet, but does that say only 32-bit Intel? 32-bit platforms are still interesting in the ARM world...

marc-hb · 2020-08-31T18:44:25Z

Not making a subscription yet,

You should :-) Available for free in 1 week. The slides are already available:
https://linuxplumbersconf.org/event/7/contributions/655/ SoC support lifecycle in the kernel
https://linuxplumbersconf.org/event/7/contributions/665/ Planning code obsolescence

but does that say only 32-bit Intel? 32-bit platforms are still interesting in the ARM world...

This page and discussions are actually mainly about Linux, ARM and the next decades. Not everyone agrees but the least disagreement seems to be about Linux slowly but surely becoming a 64bit OS.

marc-hb · 2020-09-01T03:49:59Z

Same player shoots again in #2413

plbossart requested review from ranj063 and singalsu as code owners August 27, 2020 18:13

plbossart requested a review from RDharageswari August 27, 2020 18:13

plbossart force-pushed the fix/ro-control-compilation-warning branch from 87410f5 to f021254 Compare August 27, 2020 19:45

lyakh requested changes Aug 28, 2020

View reviewed changes

kv2019i reviewed Aug 28, 2020

View reviewed changes

marc-hb mentioned this pull request Aug 28, 2020

.travis.yml: switch to nproc and use MAKEFLAGS to avoid repetition. #2405

Merged

plbossart force-pushed the fix/ro-control-compilation-warning branch 2 times, most recently from 58f779e to e1cc344 Compare August 28, 2020 21:02

plbossart force-pushed the fix/ro-control-compilation-warning branch from e1cc344 to 2508038 Compare August 28, 2020 22:09

fixup! ASoC: SOF: fix range checks

366e03c

The mix of integer and size_t generates different results in 32- and 64 bit mode, cast to size_t and use %zu format. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart requested review from bardliao and juimonen as code owners August 28, 2020 22:32

plbossart changed the title ~~fixup! ASoC: SOF: Implement snd_sof_bytes_ext_volatile_get kcontrol IO~~ Fix i386 compilation Aug 28, 2020

plbossart requested review from kv2019i, lyakh and marc-hb August 28, 2020 22:34

plbossart merged commit e42535d into thesofproject:topic/sof-dev Aug 28, 2020

marc-hb reviewed Aug 28, 2020

View reviewed changes

sound/soc/sof/control.c Outdated

Copy link

Collaborator

marc-hb Aug 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, I was misled by the error message in topology.c which you hadn't changed yet.

Fix i386 compilation #2396

Fix i386 compilation #2396

Uh oh!

Conversation

plbossart commented Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart commented Aug 27, 2020

Uh oh!

plbossart commented Aug 27, 2020

Uh oh!

marc-hb commented Aug 28, 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!

marc-hb Aug 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marc-hb Aug 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart commented Aug 28, 2020

Uh oh!

marc-hb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marc-hb commented Aug 31, 2020

Uh oh!

paulstelian97 commented Aug 31, 2020

Uh oh!

marc-hb commented Aug 31, 2020

Uh oh!

marc-hb commented Sep 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

plbossart commented Aug 27, 2020 •

edited

Loading

marc-hb Aug 28, 2020 •

edited

Loading

marc-hb Aug 28, 2020 •

edited

Loading