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.hu...@embedded-brains.de
PGP : Public key available on request.
Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
