[Pixman] warnings in pixman 0.21.4

Rolland Dudemaine rolland at ghs.com
Mon Jan 24 01:51:39 PST 2011 

Hi,

Granted, the patch was somewhat too big and contained many things.

Attached the patch splitted in five parts :
- *fence_prototype.diff resolves the incorrect fence_malloc prototype.
It also correctly protects onglobal_msg and on_alarm behind
HAVE_SIGACTION and HAVE_ALARM, since it's otherwise useless and
unreferenced.  If you still want to split these further into separate
patches, feel free to do so, this is trivial enough to do.

- *max_vx.diff corrects the initialization of max_vx. Contrary to what
you mention, the variable is declared in a local block, so the scope of
the variable is restricted to that block, regardless of whether it is
part of a macro or not. So the compiler is correct saying the value may
be used before being initialized. Also, INT32_MAX is a safe value,
because pixman-compiler.h defines it. INT32_MAX is the right value to
use since max_vx is a clamping value passed on to a callee. 0 would just
clamp down to 0, which would be plainly incorrect.

- *return_values.diff defines return values where missing. I added it as
a general return value, you may want to move that inside the switch
statement right above, this is really a coding style issue.

- *set_but_not_used.diff removes useless variable declarations. This can
only result in more efficient code, as these variables where sometimes
assigned, but their values were never used.

- *correct_dimmy_PIXMAN_types.diff is the most complex part of the
patches. My compiler was producing a number of warnings due to incorrect
uses of int instead of the correct corresponding pixman_*_t type. Also,
it was complaining (rightfully) that some values were passed as
pixman_format_code_t that were not defined in that enum. The best
resolution I could come up with was to logically move PIXMAN_null,
PIXMAN_any and the related dummy pixmap types to the enum, so that it is
possible to use them as argument where a pixman_format_code_t argument
is required. (Note : the attached patch contains one more correction for
blitters-test.c that I did not correct previously)

All tests pass on target.
FYI, the compiler is the Green Hills Software MULTI compiler, and the
target OS is the INTEGRITY RTOS.
Last, compilation log before patches are resolved is also included for
your reference (fence_malloc prototype was already corrected otherwise
it would not build).

Cheers,

--Rolland


Le 01/22/2011 11:32 PM, Siarhei Siamashka a écrit :
> On Saturday 22 January 2011 21:37:18 Rolland Dudemaine wrote:
>> Hi,
>>
>> Attached is a diff that resolves a number of warnings I encountered when
>> porting pixman over to my RTOS.
>> Most examples compile cleanly now.
>> I had to move the definitions of PIXMAN_null, PIXMAN_any and related so
>> they become type-safe, otherwise my compiler gives a enumeration to int
>> cast warning. It is cleaner that way anyway.
> Hi,
>
> Thanks for raising this issue and for a patch proposal. Making sure that the
> code is free of warnings with different compilers typically improves quality.
>
> However it might be better to split the patch into separate parts, for each 
> type of problem separately. For example, elimination of dead/unused code is
> obvious and safe. The other changes may (or may not) need more careful review.
>
> More details would be also welcome. For example, what kind of compiler was used
> and what do these warnings look like?
>
> I also have been playing with clang static analyzer just yesterday, and it
> also spits quite a lot of warnings too (mostly about dead/unused code):
> http://people.freedesktop.org/~siamashka/files/20110122/pixman-0.21.4-clang/
>
> Regarding some parts of your patch:
>
> ---
>
> +++ pixman-0.21.4/pixman/pixman-fast-path.h     2011-01-22 19:53:12.000000000 
> -    pixman_fixed_t max_vx = max_vx; /* suppress uninitialized variable warning 
> */
> +    pixman_fixed_t max_vx = INT32_MAX; /* suppress uninitialized variable 
> warning */
>
> This indeed makes a lot of sense to fix. This code is using self assignment to
> suppress the use of uninitialized variable warning, but this is apparently not
> quite portable and gcc-specific. Moreover, such code is not encouraged in gcc
> itself nowadays  ('-Winit-self' is a search keyword for more information).
>
> In pixman, this variable may be passed uninitialized as an argument for inline
> function, but is not used in the inline function itself. Strictly speaking,
> this behaviour is very likely wrong according to C standard even though it
> appears to work fine in practice with the current compilers and cpu
> architectures. I think it might be possible to rearrange the code to actually
> pass optional arguments to the inline function as a pointer to some structure,
> where some of the fields may be uninitialized if not needed. But just
> initializing 'max_vx' to something should work for now.
>
> But INT32_MAX is not a good choice for initializing this variable, I think 0
> would be better. In the case if the compiler can't see that this value is not
> actually used and optimize it out, arbitrary magic constants are handled less
> efficient on RISC processors because not every constant can be used as an
> immediate operand. For example, compiling the following functions for MIPS:
>
> int f1(void)
> {
>     return 2147483647;
> }
>
> int f2(void)
> {
>     return 0;
> }
>
> Results in the following code:
>
> 00000000 <f1>:
>    0:   3c027fff        lui     v0,0x7fff
>    4:   03e00008        jr      ra
>    8:   3442ffff        ori     v0,v0,0xffff
>
> 0000000c <f2>:
>    c:   03e00008        jr      ra
>   10:   00001021        move    v0,zero
>
> Using zero as an integer constant value is clearly liked better by MIPS
> processors.
>
> Anyway, I have some more changes to this code coming and will handle this issue
> too. Especially considering that it is also flagged by clang static analyzer.
> Thanks.
>
> ---
>
> Regarding another part of the patch:
>
>  void *
> -fence_malloc (uint32_t len)
> +fence_malloc (int64_t len)
>
> Ugh, that is a clear bug and the fix looks correct. I think we should push it
> to pixman git.
>
> ---
>
> I still did not look at the other parts of the patch closely, and the other
> people may also provide some comments.
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pixman-0.21.4_correct_dummy_PIXMAN_types.diff
Type: text/x-patch
Size: 6415 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/pixman/attachments/20110124/4665f868/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pixman-0.21.4_max_vx.diff
Type: text/x-patch
Size: 675 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/pixman/attachments/20110124/4665f868/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pixman-0.21.4_return_values.diff
Type: text/x-patch
Size: 1503 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/pixman/attachments/20110124/4665f868/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pixman-0.21.4_set_but_not_used.diff
Type: text/x-patch
Size: 2359 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/pixman/attachments/20110124/4665f868/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pixman-0.21.4_compile.log
Type: text/x-log
Size: 56279 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/pixman/attachments/20110124/4665f868/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pixman-0.21.4_fence_prototype.diff
Type: text/x-patch
Size: 595 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/pixman/attachments/20110124/4665f868/attachment-0011.bin>

More information about the Pixman mailing list