[PATCH] jffs, clang: avoid some warnings of used uninitialized ptr
Daniel Hellstrom
daniel at gaisler.com
Mon Oct 8 14:06:23 UTC 2018
On 2018-10-05 15:42, Sebastian Huber wrote:
> 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.
>
I agree. Thanks for pushing it into rtems. I can confirm latest master
now builds and the 5 jffs2 previously failing with clang now runs on
single-core gr712rc BSP.
/Daniel
More information about the devel
mailing list