Bug 54867 - bug in r300 compiler
Summary: bug in r300 compiler
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/Gallium/r300 (show other bugs)
Version: git
Hardware: All Linux (All)
: medium normal
Assignee: David Heidelberg (okias)
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-13 14:19 UTC by Tomasz P.
Modified: 2013-10-25 14:03 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
s/signed/int/ (1.04 KB, patch)
2013-05-31 02:22 UTC, Chistopher Krakowiak
Details | Splinter Review
0001-r300g-compiler-Fix-unsigned-comparison-with-less-tha.patch (1.12 KB, patch)
2013-10-07 16:09 UTC, David Heidelberg (okias)
Details | Splinter Review

Description Tomasz P. 2012-09-13 14:19:44 UTC
Orginally posted on mailing list:

In playing with Coccinelle, I discovered a signed/unsigned bug in
radeon_rename_regs.c:rc_rename_regs.

unsigned new_index;
unsigned writemask;
struct rc_variable * var = var_ptr->Item;

if (var->Inst->U.I.DstReg.File != RC_FILE_TEMPORARY) {
        continue;
}

new_index = rc_find_free_temporary_list(c, used, used_length,
                                                RC_MASK_XYZW);
if (new_index < 0) {
        rc_error(c, "Ran out of temporary registers\n");
        return;
}

unsigned new_index is compared with < 0.

I don't know the code, but I can't imagine that you'd need an unsigned
to represent a register index value.

Matt Turner
Comment 1 Chistopher Krakowiak 2013-05-31 02:22:18 UTC
Created attachment 80074 [details] [review]
s/signed/int/
Comment 2 Tom Stellard 2013-06-03 13:31:28 UTC
Comment on attachment 80074 [details] [review]
s/signed/int/

Review of attachment 80074 [details] [review]:
-----------------------------------------------------------------

This patch looks good to me, but for the commit message, you need to wrap long lines to 80 or fewer characters (I actually wrap to 75, but I'm not sure what the standard convention is) and re-organize the commit message in the form of:

Code area: Brief description

Long description (if necessary)

Link to fixed bugs

For this patch, it should be something like:


r300g/compiler: Fix unsigned comparison with less than zero

rc_find_free_temporary_list() returns signed integer (in case of lack of free temporary registersreturns -1), so new_index in radeon_rename_regs() should be signed.

https://bugs.freedesktop.org/show_bug.cgi?id=54867
Comment 3 David Heidelberg (okias) 2013-10-04 20:02:15 UTC
So, still not pushed in today git, can someone push this small fix?
Comment 4 Tom Stellard 2013-10-07 13:41:57 UTC
(In reply to comment #3)
> So, still not pushed in today git, can someone push this small fix?

I can push it if you provide an updated patch with a proper commit message.
Comment 5 David Heidelberg (okias) 2013-10-07 16:09:51 UTC
Created attachment 87247 [details] [review]
0001-r300g-compiler-Fix-unsigned-comparison-with-less-tha.patch

Hi Tom,

sending correctly formated commit message.

David
Comment 6 Fabio Pedretti 2013-10-15 08:41:40 UTC
Ping?


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.