Bug 80266

Summary: Many instances of 1<<31, which is undefined in C99
Product: Mesa Reporter: Vittorio <zeccav>
Component: Mesa coreAssignee: mesa-dev
Status: RESOLVED MOVED QA Contact:
Severity: minor    
Priority: medium    
Version: 10.2   
Hardware: x86-64 (AMD64)   
OS: Linux (All)   
URL: http://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html
Whiteboard:
i915 platform: i915 features:

Description Vittorio 2014-06-20 05:46:58 UTC
In tgsi_ureg.c:1498
"if (ureg->vs_inputs[i/32] & (1 << (i%32))) {"
when i==31 then 1 << 31 may be computed.
With gcc option -std=c99 this is wrong because the result cannot be int.

Perhaps the right instruction is
"if (ureg->vs_inputs[i/32] & ((unsigned) 1 << (i%32))) {"
Comment 1 Ilia Mirkin 2014-06-20 05:48:55 UTC
(In reply to comment #0)
> In tgsi_ureg.c:1498
> "if (ureg->vs_inputs[i/32] & (1 << (i%32))) {"
> when i==31 then 1 << 31 may be computed.
> With gcc option -std=c99 this is wrong because the result cannot be int.
> 
> Perhaps the right instruction is
> "if (ureg->vs_inputs[i/32] & ((unsigned) 1 << (i%32))) {"

Where is -std=c99 coming from? Also I'd guess the better thing is 1U instead of (unsigned)1 :)

Feel free to send a patch btw.
Comment 2 Vittorio 2014-06-20 09:50:36 UTC
Yes I agree that 1U is better. My C knowledge is limited, I am more
interested into Fortran.

-std=c99 is in CFLAGS in the Makefile produced by configure and configure.ac.

Not much familiar with patch, please help yourself.
Comment 3 Vittorio 2014-06-20 11:53:07 UTC
For the same reason I believe that in uniforms.c:79
"if (prog->SamplersUsed & (1 << s)) {" should be
"if (prog->SamplersUsed & (1U << s)) {".
This is running glxgears.
Comment 4 Vittorio 2014-06-20 18:12:44 UTC
Also in matrix.c:1156 the line
"#define ONE(x)  (1 <<(x+16))"
should be
"#define ONE(x)  (1U<<(x+16))"
because ONE(15) later, as at line 1226, produces 16 bits left shift of 65536,
which is bigger than int.
This is running teapot.
Comment 5 Ilia Mirkin 2014-06-20 18:18:24 UTC
I'm not sure why you're filing these against nouveau... I'm sure there are _tons_ of these throughout mesa...

Does gcc warn about this? Are you _sure_ that (signed int)1 << 31 is illegal in C99?
Comment 6 Patrick Baggett 2014-06-20 18:45:44 UTC
I'm sure that technically shifting any bits into the sign bit of a signed number produces undefined behavior according to C99, so yes, (1<<31) technically is undefined. As a matter of practicality, on basically any CPU, 1<<31 will produce a valid negative number, but the compiler sees it as trying to assign the constant 2147483648 to a 32-bit signed container, which is outside of the 2^31-1 range that it can accept.

In the C abstract machine, this would be a serious problem. Obviously you can make such an assignment in hardware, but the result (when interpreted as signed) is not a value of 2147483648, but a value of -2147483648, which is why the compiler gives a warning. It's weird to read that (1<<31) is less than 0. If you don't really mean to assign a value of -2147483648 or 2147483648 and just want a bit pattern, using unsigned is almost always the way to go.

I know that Solaris Studio happens to give a warning for the same case, if that means anything.
Comment 7 Vittorio 2014-06-20 19:03:13 UTC
In m_matrix.c:1215
"   if (m[15] == 1.0F) mask |= (1<<31);"
should be
"   if (m[15] == 1.0F) mask |= (1U<<31);"
because 1<<31 does not fit in int.
From teapot.
Comment 8 Vittorio 2014-06-20 19:12:06 UTC
(In reply to comment #5)
> I'm not sure why you're filing these against nouveau... I'm sure there are
> _tons_ of these throughout mesa...
> 
> Does gcc warn about this? Are you _sure_ that (signed int)1 << 31 is illegal
> in C99?
Only now I am reading this note.
I compiled mesa with gcc -fsanitize=undefined option, gcc runtime is issuing
several, not tons, of runtime error messages, it seems to me that 
(signed int) 1 << 31 is undefined if computed as int.
The runtime error messages only appear with the -std=c99 option.
Comment 9 Vittorio 2014-06-20 19:16:29 UTC
(In reply to comment #6)
> I'm sure that technically shifting any bits into the sign bit of a signed
> number produces undefined behavior according to C99, so yes, (1<<31)
> technically is undefined. As a matter of practicality, on basically any CPU,
> 1<<31 will produce a valid negative number, but the compiler sees it as
> trying to assign the constant 2147483648 to a 32-bit signed container, which
> is outside of the 2^31-1 range that it can accept.
> 
> In the C abstract machine, this would be a serious problem. Obviously you
> can make such an assignment in hardware, but the result (when interpreted as
> signed) is not a value of 2147483648, but a value of -2147483648, which is
> why the compiler gives a warning. It's weird to read that (1<<31) is less
> than 0. If you don't really mean to assign a value of -2147483648 or
> 2147483648 and just want a bit pattern, using unsigned is almost always the
> way to go.
> 
> I know that Solaris Studio happens to give a warning for the same case, if
> that means anything.

Thanks for the appreciated explanation.
Maybe substituting 1<<31 with 1U<<31 clears any undefined behaviour on the many architectures where mesa runs.
Comment 10 Patrick Baggett 2014-06-20 19:22:34 UTC
(In reply to comment #5)
> Does gcc warn about this? Are you _sure_ that (signed int)1 << 31 is illegal
> in C99?

And as tedious as this is to mention, C99 section 6.5.7.4 does state this is undefined behavior. I know the "C abstract machine" is far removed from real problems with drivers and whatnot -- and I appreciate the awesome work you all do on Mesa. I just wanted to clarify that there is spec support for this claim.
Comment 11 Vittorio 2014-06-20 20:12:34 UTC
Another one in nouveau_fence.c:170
"#define NOUVEAU_FENCE_MAX_SPINS (1 << 31)"
should be
"#define NOUVEAU_FENCE_MAX_SPINS (1U<< 31)"
Comment 12 Vittorio 2014-06-21 09:16:50 UTC
In texstore.c lines 1281 and 1340 255 << 24 is computed in PACK_COLOR_8888.
Comment 13 Vittorio 2014-07-04 05:42:33 UTC
More issues:
u_pack_color.h:365
"uc->ui = (a << 24) | (r << 16) | (g << 8) | b;"
should be
"uc->ui = ((unsigned int)a << 24) | (r << 16) | (g << 8) | b;"

m_matrix.c line 1156
"#define ONE(x)  (1<<(x+16))"
should be
"#define ONE(x)  (1U<<(x+16))"
because of ONE(15).
At line 1215
"if (m[15] == 1.0F) mask |= (1<<31);"
should be
"if (m[15] == 1.0F) mask |= (1U<<31);"
Comment 14 Eero Tamminen 2014-07-15 09:14:08 UTC
(In reply to comment #10)
> And as tedious as this is to mention, C99 section 6.5.7.4 does state this is
> undefined behavior. I know the "C abstract machine" is far removed from real
> problems with drivers and whatnot -- and I appreciate the awesome work you
> all do on Mesa. I just wanted to clarify that there is spec support for this
> claim.

LLVM compiler site has 3 article series on issues with undefined behavior (see URL field), it's a very good read.

Coccinelle might be able to help detection and patching of issues like this: http://lwn.net/Articles/380835/
Comment 15 Michel Dänzer 2014-07-16 09:07:01 UTC
Please submit patches fixing these issues to the mesa-dev mailing list, instead of waiting here for anyone else to do it.
Comment 16 Vittorio 2014-07-19 05:48:25 UTC
If comment #15 is directed to me,
I believe I already did my part by exposing this issue on mesa bugzilla.
The C99 standard clearly states that such shifts are undefined and
then against the standard.
Mesa works on many platforms and it should be at least standard
compliant to ensure its correct execution.
Maybe this undefined behaviour provokes some glitches in mesa, maybe not.
I have not the expertise nor the time to submit patches.
Are you telling me nobody is going to fix these bugs?
If you are happy with that I am happy.
Comment 17 GitLab Migration User 2019-09-18 20:23:21 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/mesa/mesa/issues/980.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.