Nouveau - Jun 2014 - [Bug 80266] New: Undefined operation in tgsi_ureg.c left shift of 1 by 31 places cannot be represented in type 'int'

https://bugs.freedesktop.org/show_bug.cgi?id=80266

          Priority: medium
            Bug ID: 80266
          Assignee: nouveau at lists.freedesktop.org
           Summary: Undefined operation in tgsi_ureg.c left shift of 1 by
                    31 places cannot be represented in type 'int'
          Severity: minor
    Classification: Unclassified
                OS: Linux (All)
          Reporter: zeccav at gmail.com
          Hardware: x86-64 (AMD64)
            Status: NEW
           Version: 10.2
         Component: Drivers/DRI/nouveau
           Product: Mesa

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))) {"

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20140620/8de391b6/attachment.html>

https://bugs.freedesktop.org/show_bug.cgi?id=80266

--- Comment #1 from Ilia Mirkin <imirkin at alum.mit.edu> ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20140620/0e4f37be/attachment.html>

https://bugs.freedesktop.org/show_bug.cgi?id=80266

--- Comment #2 from Vittorio <zeccav at gmail.com> ---
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20140620/4df39761/attachment.html>

https://bugs.freedesktop.org/show_bug.cgi?id=80266

--- Comment #3 from Vittorio <zeccav at gmail.com> ---
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20140620/0c542fcd/attachment.html>

https://bugs.freedesktop.org/show_bug.cgi?id=80266

--- Comment #4 from Vittorio <zeccav at gmail.com> ---
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20140620/d5e3bc23/attachment.html>

https://bugs.freedesktop.org/show_bug.cgi?id=80266

--- Comment #5 from Ilia Mirkin <imirkin at alum.mit.edu> ---
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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20140620/85a8213f/attachment.html>

https://bugs.freedesktop.org/show_bug.cgi?id=80266

--- Comment #6 from Patrick Baggett <baggett.patrick at gmail.com> ---
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20140620/5f537d98/attachment-0001.html>

https://bugs.freedesktop.org/show_bug.cgi?id=80266

--- Comment #7 from Vittorio <zeccav at gmail.com> ---
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.
-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20140620/f4c3e856/attachment.html>

https://bugs.freedesktop.org/show_bug.cgi?id=80266

--- Comment #8 from Vittorio <zeccav at gmail.com> ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20140620/d5d41259/attachment.html>

https://bugs.freedesktop.org/show_bug.cgi?id=80266

--- Comment #9 from Vittorio <zeccav at gmail.com> ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20140620/c401573f/attachment.html>

https://bugs.freedesktop.org/show_bug.cgi?id=80266

Ilia Mirkin <imirkin at alum.mit.edu> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|nouveau at lists.freedesktop.o |mesa-dev at
lists.freedesktop.
                   |rg                          |org
            Summary|Undefined operation in      |Many instances of 1<<31,
                   |tgsi_ureg.c left shift of 1 |which is undefined in C99
                   |by 31 places cannot be      |
                   |represented in type 'int'   |
          Component|Drivers/DRI/nouveau         |Mesa core

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20140620/fbed131b/attachment.html>