[PATCH] jffs, clang: avoid some warnings of used uninitialized ptr

Sebastian Huber sebastian.huber at embedded-brains.de
Fri Oct 5 13:42:47 UTC 2018


On 05/10/2018 15:33, Daniel Hellstrom wrote:
> On 2018-10-05 14:01, Sebastian Huber wrote:
>> On 05/10/2018 13:43, Daniel Hellstrom wrote:
>>> On 2018-10-05 13:01, Sebastian Huber wrote:
>>>> On 05/10/2018 12:44, Daniel Hellstrom wrote:
>>>>>
>>>>> On 2018-10-05 09:41, Sebastian Huber wrote:
>>>>>> On 05/10/2018 09:38, Daniel Hellstrom wrote:
>>>>>>>
>>>>>>> On 2018-10-05 09:34, Sebastian Huber wrote:
>>>>>>>> On 05/10/2018 09:25, Daniel Hellstrom wrote:
>>>>>>>>> On 2018-10-05 09:12, Sebastian Huber wrote:
>>>>>>>>>> On 05/10/2018 08:57, Daniel Hellstrom wrote:
>>>>>>>>>>> This fixes the following test failures on LEON3 UP/SMP when
>>>>>>>>>>> built using clang compiler:
>>>>>>>>>>>   * fsjffs2gc01
>>>>>>>>>>>   * jffs2_fserror
>>>>>>>>>>>   * jffs2_fspermission
>>>>>>>>>>>   * jffs2_fsrdwr
>>>>>>>>>>>   * jffs2_fstime
>>>>>>>>>>
>>>>>>>>>> The problem is probably in the 
>>>>>>>>>> rbtree_postorder_for_each_entry_safe(). Coverity Scan has 
>>>>>>>>>> also an issue with this macro. It is on my todo list. Please 
>>>>>>>>>> do not check in this patch.
>>>>>>>>> Ok I will not push it. Yes I was also thinking so. To use 
>>>>>>>>> offsetof() macro could be an option maybe, however the node 
>>>>>>>>> type input is not known to the 
>>>>>>>>> rbtree_postorder_for_each_entry_safe() function even though it 
>>>>>>>>> only used from jffs2 code. 
>>>>>>>>
>>>>>>>> The Linux code uses typeof. We could do this also:
>>>>>>>>
>>>>>>>> diff --git a/cpukit/include/linux/rbtree.h 
>>>>>>>> b/cpukit/include/linux/rbtree.h
>>>>>>>> index 53c777e8c1..8fc575240f 100644
>>>>>>>> --- a/cpukit/include/linux/rbtree.h
>>>>>>>> +++ b/cpukit/include/linux/rbtree.h
>>>>>>>> @@ -126,12 +126,12 @@ static inline struct rb_node *rb_parent( 
>>>>>>>> struct rb_node *node )
>>>>>>>>    for ( \
>>>>>>>>      node = _RBTree_Postorder_first( \
>>>>>>>>        (RBTree_Control *) root, \
>>>>>>>> -      (size_t) ( (char *) &node->field - (char *) node ) \
>>>>>>>> +      offsetof( __typeof__( *node ), field ) \
>>>>>>>>      ); \
>>>>>>>>      node != NULL && ( \
>>>>>>>>        next = _RBTree_Postorder_next( \
>>>>>>>>          &node->field, \
>>>>>>>> -        (size_t) ( (char *) &node->field - (char *) node ) \
>>>>>>>> +        offsetof( __typeof__( *node ), field ) \
>>>>>>>>        ), \
>>>>>>>>        node != NULL \
>>>>>>>>      ); \
>>>>>>>>
>>>>>>>> It shouldn't be a big problem since at least GCC and clang 
>>>>>>>> supports it. It is also a standard C++ operator. It is already 
>>>>>>>> used in
>>>>>>>>
>>>>>>>> cpukit/libfs/src/jffs2/include/linux/list.h
>>>>>>>
>>>>>>> Ok, I will give it a try.
>>>>>>>
>>>>>>
>>>>>> Thanks, please try this patch:
>>>>>>
>>>>>> https://lists.rtems.org/pipermail/devel/2018-October/023181.html
>>>>>
>>>>> Yes I can confirm it fixed all the 5 tests on gr712rc with clang. 
>>>>> It also removed the two warnings in jffs2 code related.
>>>>>
>>>>> I have updated my patch to only include the third warning fix then:
>>>>>
>>>>> ---
>>>>>  cpukit/libfs/src/jffs2/src/readinode.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/cpukit/libfs/src/jffs2/src/readinode.c 
>>>>> b/cpukit/libfs/src/jffs2/src/readinode.c
>>>>> index 519b0d6..ecdf335 100644
>>>>> --- a/cpukit/libfs/src/jffs2/src/readinode.c
>>>>> +++ b/cpukit/libfs/src/jffs2/src/readinode.c
>>>>> @@ -35,7 +35,7 @@ static int check_node_data(struct jffs2_sb_info 
>>>>> *c, struct jffs2_tmp_dnode_info
>>>>>      struct jffs2_raw_node_ref *ref = tn->fn->raw;
>>>>>      int err = 0, pointed = 0;
>>>>>      struct jffs2_eraseblock *jeb;
>>>>> -    unsigned char *buffer;
>>>>> +    unsigned char *buffer = NULL;
>>>>>      uint32_t crc, ofs, len;
>>>>>      size_t retlen; 
>>>>
>>>> What is this for a warning? Some sort of uninitialized variable 
>>>> warning? This code is identical to the Linux upstream. This is 
>>>> probably a clang bug.
>>>>
>>> buffer will never be uninitialized from what I see, so yes maybe its 
>>> a clang bug. This is the warning:
>>>
>>>
>>> sparc-gaisler-rtems5-clang --pipe -DHAVE_CONFIG_H   -I.. 
>>> -I/home/daniel/git/rtems/rcc-1.3/rtems/build/gr712rc_drvmgr_test_up/sparc-gaisler-rtems5/c/gr712rc/include 
>>> -I/home/daniel/git/rtems/rcc-1.3/rtems/kernel/cpukit/include 
>>> -I/home/daniel/git/rtems/rcc-1.3/rtems/kernel/cpukit/score/cpu/sparc/include 
>>> -I/home/daniel/git/rtems/rcc-1.3/rtems/kernel/cpukit/libnetworking 
>>> -I/home/daniel/git/rtems/rcc-1.3/rtems/kernel/c/src/../../cpukit/libfs/src/jffs2/include 
>>> -Wno-pointer-sign -mcpu=gr712rc -O2 -g -ffunction-sections 
>>> -fdata-sections -Wall -Wmissing-prototypes 
>>> -Wimplicit-function-declaration -Wstrict-prototypes -Wnested-externs 
>>> -MT src/jffs2/src/libjffs2_a-readinode.o -MD -MP -MF 
>>> src/jffs2/src/.deps/libjffs2_a-readinode.Tpo -c -o 
>>> src/jffs2/src/libjffs2_a-readinode.o `test -f 
>>> 'src/jffs2/src/readinode.c' || echo 
>>> '/home/daniel/git/rtems/rcc-1.3/rtems/kernel/c/src/../../cpukit/libfs/'`src/jffs2/src/readinode.c 
>>>
>>> /home/daniel/git/rtems/rcc-1.3/rtems/kernel/c/src/../../cpukit/libfs/src/jffs2/src/readinode.c:80:6: 
>>> warning: variable 'buffer' is used uninitialized whenever 'if' 
>>> condition is false [-Wsometimes-uninitialized]
>>>         if (!pointed) {
>>>             ^~~~~~~~
>>
>> We have
>>
>> int pointed = 0;
>>
>> and the address of pointed is not taken, so this condition will be 
>> always true. I am not sure what the constraints of the 
>> -Wsometimes-uninitialized warnings are. Maybe they should trigger in 
>> case code modifications elsewhere might lead to the offending 
>> condition. Coverity Scan doesn't complain about this one.
>>
>> Please have a look at this patch:
>>
>> https://lists.rtems.org/pipermail/devel/2018-October/023190.html
>>
>> In contrast to the buffer = NULL initialization this patch entirely 
>> avoids the problematic path which must the compiler follow to deduces 
>> this warning.
>
> It works too, the size and disassembly is the same in both cases with 
> clang (optimizations is turned on though). Apart from the code getting 
> harder to read, I believe it would break if __ECOS would be undefined 
> since pointed is used then. Maybe an #errror or an assert within the 
> #ifndef __ECOS should be added to signal that the code would break 
> otherwise. 

Thanks for testing. If you undefine __ECOS a lot more things will break. 
If you use the buffer = NULL initialization and a new generation of 
static analysis tools starts to look at cyg_crc32_accumulate() it will 
notice undefined behaviour again.

-- 
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax     : +49 89 189 47 41-09
E-Mail  : sebastian.huber at embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.




More information about the devel mailing list