Hello, I work on updating mandoc to 1.14.5 on illumos and there are few errors (warnings treated as errors) when building mandoc with GCC 4.4.4 and 7.4.0: + /opt/gcc/4.4.4/bin/gcc -fident -finline -fno-inline-functions -fno-builtin -fno-asm -fdiagnostics-show-option -nodefaultlibs -D__sun -O -m32 -Wall -Wextra -Werror -Wno-missing-braces -Wno-sign-compare -Wno-unknown-pragmas -Wno-unused-parameter -Wno-missing-field-initializers -Wno-array-bounds -std=gnu99 -fno-inline-small-functions -fno-inline-functions-called-once -fno-ipa-cp -D_TS_ERRNO -DOSNAME="illumos" -D_FILE_OFFSET_BITS=64 -nostdinc -I. -include fts.h -I/usr/include -c -o mdoc_term.o /export/home/newman/ws/oi-userland/components/openindiana/illumos-gate/illumos-gate/usr/src/cmd/mandoc/mdoc_term.c cc1: warnings being treated as errors /export/home/newman/ws/oi-userland/components/openindiana/illumos-gate/illumos-gate/usr/src/cmd/mandoc/mdoc_term.c: In function 'print_mdoc_node': /export/home/newman/ws/oi-userland/components/openindiana/illumos-gate/illumos-gate/usr/src/cmd/mandoc/mdoc_term.c:310: error: 'act' may be used uninitialized in this function [-Wuninitialized] + /usr/gcc/7/bin/gcc -fident -finline -fno-inline-functions -fno-builtin -fno-asm -fdiagnostics-show-option -nodefaultlibs -D__sun -O -m32 -Wall -Wextra -Werror -Wno-missing-braces -Wno-sign-compare -Wno-unknown-pragmas -Wno-unused-parameter -Wno-missing-field-initializers -Wno-array-bounds -std=gnu99 -fno-inline-small-functions -fno-inline-functions-called-once -fno-ipa-cp -fno-ipa-icf -fno-clone-functions -fno-reorder-functions -D_TS_ERRNO -DOSNAME="illumos" -D_FILE_OFFSET_BITS=64 -nostdinc -I. -include fts.h -I/usr/include -c -o /tmp/nightly.tmpdir.3713/cw.YdaGE4/cwYdaWE4.o /export/home/newman/ws/oi-userland/components/openindiana/illumos-gate/illumos-gate/usr/src/cmd/mandoc/mdoc_term.c /export/home/newman/ws/oi-userland/components/openindiana/illumos-gate/illumos-gate/usr/src/cmd/mandoc/mdoc_term.c: In function 'print_mdoc_node': /export/home/newman/ws/oi-userland/components/openindiana/illumos-gate/illumos-gate/usr/src/cmd/mandoc/mdoc_term.c:418:10: error: 'act' may be used uninitialized in this function [-Werror=maybe-uninitialized] if (act->post == NULL || n->flags & NODE_ENDED) ~~~^~~~~~ cc1: all warnings being treated as errors *** Error code 1 + /opt/gcc/4.4.4/bin/gcc -fident -finline -fno-inline-functions -fno-builtin -fno-asm -fdiagnostics-show-option -nodefaultlibs -D__sun -O -m32 -Wall -Wextra -Werror -Wno-missing-braces -Wno-sign-compare -Wno-unknown-pragmas -Wno-unused-parameter -Wno-missing-field-initializers -Wno-array-bounds -std=gnu99 -fno-inline-small-functions -fno-inline-functions-called-once -fno-ipa-cp -D_TS_ERRNO -DOSNAME="illumos" -D_FILE_OFFSET_BITS=64 -nostdinc -I. -include fts.h -I/usr/include -c -o read.o /export/home/newman/ws/oi-userland/components/openindiana/illumos-gate/illumos-gate/usr/src/cmd/mandoc/read.c cc1: warnings being treated as errors /export/home/newman/ws/oi-userland/components/openindiana/illumos-gate/illumos-gate/usr/src/cmd/mandoc/read.c: In function 'mparse_buf_r': /export/home/newman/ws/oi-userland/components/openindiana/illumos-gate/illumos-gate/usr/src/cmd/mandoc/read.c:147: error: 'lastln' may be used uninitialized in this function [-Wuninitialized] + /opt/gcc/4.4.4/bin/gcc -fident -finline -fno-inline-functions -fno-builtin -fno-asm -fdiagnostics-show-option -nodefaultlibs -D__sun -O -m32 -Wall -Wextra -Werror -Wno-missing-braces -Wno-sign-compare -Wno-unknown-pragmas -Wno-unused-parameter -Wno-missing-field-initializers -Wno-array-bounds -std=gnu99 -fno-inline-small-functions -fno-inline-functions-called-once -fno-ipa-cp -D_TS_ERRNO -DOSNAME="illumos" -D_FILE_OFFSET_BITS=64 -nostdinc -I. -include fts.h -I/usr/include -c -o term.o /export/home/newman/ws/oi-userland/components/openindiana/illumos-gate/illumos-gate/usr/src/cmd/mandoc/term.c cc1: warnings being treated as errors /export/home/newman/ws/oi-userland/components/openindiana/illumos-gate/illumos-gate/usr/src/cmd/mandoc/term.c: In function 'term_fill': /export/home/newman/ws/oi-userland/components/openindiana/illumos-gate/illumos-gate/usr/src/cmd/mandoc/term.c:258: error: 'vn' may be used uninitialized in this function [-Wuninitialized] + /usr/gcc/7/bin/gcc -fident -finline -fno-inline-functions -fno-builtin -fno-asm -fdiagnostics-show-option -nodefaultlibs -D__sun -O -m32 -Wall -Wextra -Werror -Wno-missing-braces -Wno-sign-compare -Wno-unknown-pragmas -Wno-unused-parameter -Wno-missing-field-initializers -Wno-array-bounds -std=gnu99 -fno-inline-small-functions -fno-inline-functions-called-once -fno-ipa-cp -fno-ipa-icf -fno-clone-functions -fno-reorder-functions -D_TS_ERRNO -DOSNAME="illumos" -D_FILE_OFFSET_BITS=64 -nostdinc -I. -include fts.h -I/usr/include -c -o /tmp/nightly.tmpdir.3713/cw.QPaOX4/cwRPa4X4.o /export/home/newman/ws/oi-userland/components/openindiana/illumos-gate/illumos-gate/usr/src/cmd/mandoc/term.c /export/home/newman/ws/oi-userland/components/openindiana/illumos-gate/illumos-gate/usr/src/cmd/mandoc/term.c: In function 'term_fill': /export/home/newman/ws/oi-userland/components/openindiana/illumos-gate/illumos-gate/usr/src/cmd/mandoc/term.c:286:24: error: 'vn' may be used uninitialized in this function [-Werror=maybe-uninitialized] if (breakline || vn > vtarget) ~~~^~~~~~~~~ cc1: all warnings being treated as errors *** Error code 1 In all cases I was able to eliminate those errors by initializing respective variables to NULL. It seems that none of those errors were addressed after the 1.14.5 release. Michal -- To unsubscribe send an email to discuss+unsubscribe@mandoc.bsd.lv
Hi Michael, Michal Nowak wrote on Mon, Jun 03, 2019 at 05:28:28PM +0200: > I work on updating mandoc to 1.14.5 on illumos Appreciated, thank you! > and there are few errors (warnings treated as errors) when building > mandoc with GCC 4.4.4 and 7.4.0: [text reordered] > In all cases I was able to eliminate those errors by initializing > respective variables to NULL. In general, i object to blindly initializing variables without considering whether it makes sense logically; that is dangerous behaviour on part of the programmer because it can hide errors in the code - both existing errors and errors introduced later. > It seems that none of those errors were > addressed after the 1.14.5 release. No, because no are errors: they are all false positives. > In function 'print_mdoc_node': > error: 'act' may be used uninitialized in this function > [-Werror=maybe-uninitialized] > if (act->post == NULL || n->flags & NODE_ENDED) > ~~~^~~~~~ The pointer act is assigned to in the default case of the previous n->type switch, and it is used only in the default case of the second n->type switch. That said, the only way this pointer is used is by dereferencing it. So (pointlessly) initializing it does no harm because in case a bug is introduced later, it changes an uninitialized pointer access into a NULL pointer access which is potentially less dangerous. So in this particular case, your suggestion to hide the error is OK. > In function 'mparse_buf_r': > /export/home/newman/ws/oi-userland/components/openindiana/illumos-gate/illumos-gate/usr/src/cmd/mandoc/read.c:147: > error: 'lastln' may be used uninitialized in this function [-Wuninitialized] Another false positive: lastln is only used when firstln is already set, and whenever firstln gets set, lastln is also set at the same time. That said, it is logically clear from the purpose of the variable that initializing lastln is harmless in this case. Logically, lastln *should* be NULL as long as firstln is NULL. > In function 'term_fill': > /export/home/newman/ws/oi-userland/components/openindiana/illumos-gate/illumos-gate/usr/src/cmd/mandoc/term.c:286:24: > error: 'vn' may be used uninitialized in this function > [-Werror=maybe-uninitialized] > if (breakline || vn > vtarget) And yet another false positive. The variable vn is assigned to in all three cases of the inner switch and only used immediately afterwards. Apparently, the compiler isn't smart enough to see that the cases of the inner switch are exhaustive. Then again, there is no harm in saying so explicitly. So, i committed the three patches shown below. Can you confirm that your gcc no longer complains? Yours, Ingo Log Message: ----------- Initialize the local variable "act" in print_mdoc_node(). While there is no bug, it helps clarity, and it is also safer in this particular code because in case a bug gets introduced later, accessing a NULL pointer is less dangerous than accessing an uninitialized pointer. Michal Nowak <mnowak at startmail dot com> reported that gcc 4.4.4 and 7.4.0 on illumos throw -Wuninitialized false positives. Modified Files: -------------- mandoc: mdoc_term.c Revision Data ------------- Index: mdoc_term.c =================================================================== RCS file: /home/cvs/mandoc/mandoc/mdoc_term.c,v retrieving revision 1.372 retrieving revision 1.373 diff -Lmdoc_term.c -Lmdoc_term.c -u -p -r1.372 -r1.373 --- mdoc_term.c +++ mdoc_term.c @@ -352,6 +352,7 @@ print_mdoc_node(DECL_ARGS) * produce output. Note that some pre-handlers do so. */ + act = NULL; switch (n->type) { case ROFFT_TEXT: if (n->flags & NODE_LINE) { Log Message: ----------- Initialize the local variable "lastln" in mparse_buf_r(). While there is no bug, it logically makes sense given the meaning of the variable that lastln is NULL as long as firstln is NULL. Michal Nowak <mnowak at startmail dot com> reported that gcc 4.4.4 and 7.4.0 on illumos throw -Wuninitialized false positives. Modified Files: -------------- mandoc: read.c Revision Data ------------- Index: read.c =================================================================== RCS file: /home/cvs/mandoc/mandoc/read.c,v retrieving revision 1.212 retrieving revision 1.213 diff -Lread.c -Lread.c -u -p -r1.212 -r1.213 --- read.c +++ read.c @@ -157,7 +157,7 @@ mparse_buf_r(struct mparse *curp, struct ln.sz = 256; ln.buf = mandoc_malloc(ln.sz); ln.next = NULL; - firstln = loop = NULL; + firstln = lastln = loop = NULL; lnn = curp->line; pos = 0; inloop = 0; Log Message: ----------- Initialize the local variable "lastln" in mparse_buf_r(). While there is no bug, it logically makes sense given the meaning of the variable that lastln is NULL as long as firstln is NULL. Michal Nowak <mnowak at startmail dot com> reported that gcc 4.4.4 and 7.4.0 on illumos throw -Wuninitialized false positives. Modified Files: -------------- mandoc: read.c Revision Data ------------- Index: read.c =================================================================== RCS file: /home/cvs/mandoc/mandoc/read.c,v retrieving revision 1.212 retrieving revision 1.213 diff -Lread.c -Lread.c -u -p -r1.212 -r1.213 --- read.c +++ read.c @@ -157,7 +157,7 @@ mparse_buf_r(struct mparse *curp, struct ln.sz = 256; ln.buf = mandoc_malloc(ln.sz); ln.next = NULL; - firstln = loop = NULL; + firstln = lastln = loop = NULL; lnn = curp->line; pos = 0; inloop = 0; -- To unsubscribe send an email to discuss+unsubscribe@mandoc.bsd.lv
Thank you Ingo very much for your comments. I'll go with your patches. On 06/03/19 10:37 PM, Ingo Schwarze wrote: > Hi Michael, > > Michal Nowak wrote on Mon, Jun 03, 2019 at 05:28:28PM +0200: > >> I work on updating mandoc to 1.14.5 on illumos > > Appreciated, thank you! > >> and there are few errors (warnings treated as errors) when building >> mandoc with GCC 4.4.4 and 7.4.0: > > [text reordered] >> In all cases I was able to eliminate those errors by initializing >> respective variables to NULL. > > In general, i object to blindly initializing variables without > considering whether it makes sense logically; that is dangerous > behaviour on part of the programmer because it can hide errors in > the code - both existing errors and errors introduced later. > >> It seems that none of those errors were >> addressed after the 1.14.5 release. > > No, because no are errors: they are all false positives. > >> In function 'print_mdoc_node': >> error: 'act' may be used uninitialized in this function >> [-Werror=maybe-uninitialized] >> if (act->post == NULL || n->flags & NODE_ENDED) >> ~~~^~~~~~ > > The pointer act is assigned to in the default case of the previous > n->type switch, and it is used only in the default case of the > second n->type switch. > > That said, the only way this pointer is used is by dereferencing it. > So (pointlessly) initializing it does no harm because in case a bug > is introduced later, it changes an uninitialized pointer access into > a NULL pointer access which is potentially less dangerous. So in this > particular case, your suggestion to hide the error is OK. > >> In function 'mparse_buf_r': >> /export/home/newman/ws/oi-userland/components/openindiana/illumos-gate/illumos-gate/usr/src/cmd/mandoc/read.c:147: >> error: 'lastln' may be used uninitialized in this function [-Wuninitialized] > > Another false positive: lastln is only used when firstln is already > set, and whenever firstln gets set, lastln is also set at the same > time. That said, it is logically clear from the purpose of the > variable that initializing lastln is harmless in this case. Logically, > lastln *should* be NULL as long as firstln is NULL. > >> In function 'term_fill': >> /export/home/newman/ws/oi-userland/components/openindiana/illumos-gate/illumos-gate/usr/src/cmd/mandoc/term.c:286:24: >> error: 'vn' may be used uninitialized in this function >> [-Werror=maybe-uninitialized] >> if (breakline || vn > vtarget) > > And yet another false positive. The variable vn is assigned to in > all three cases of the inner switch and only used immediately afterwards. > > Apparently, the compiler isn't smart enough to see that the cases > of the inner switch are exhaustive. Then again, there is no harm > in saying so explicitly. > > So, i committed the three patches shown below. > > Can you confirm that your gcc no longer complains? > > Yours, > Ingo > > > Log Message: > ----------- > Initialize the local variable "act" in print_mdoc_node(). > While there is no bug, it helps clarity, and it is also safer in this > particular code because in case a bug gets introduced later, accessing > a NULL pointer is less dangerous than accessing an uninitialized pointer. > > Michal Nowak <mnowak at startmail dot com> reported that gcc 4.4.4 > and 7.4.0 on illumos throw -Wuninitialized false positives. > > Modified Files: > -------------- > mandoc: > mdoc_term.c > > Revision Data > ------------- > Index: mdoc_term.c > =================================================================== > RCS file: /home/cvs/mandoc/mandoc/mdoc_term.c,v > retrieving revision 1.372 > retrieving revision 1.373 > diff -Lmdoc_term.c -Lmdoc_term.c -u -p -r1.372 -r1.373 > --- mdoc_term.c > +++ mdoc_term.c > @@ -352,6 +352,7 @@ print_mdoc_node(DECL_ARGS) > * produce output. Note that some pre-handlers do so. > */ > > + act = NULL; > switch (n->type) { > case ROFFT_TEXT: > if (n->flags & NODE_LINE) { > > > Log Message: > ----------- > Initialize the local variable "lastln" in mparse_buf_r(). > While there is no bug, it logically makes sense given the meaning > of the variable that lastln is NULL as long as firstln is NULL. > > Michal Nowak <mnowak at startmail dot com> reported that gcc 4.4.4 > and 7.4.0 on illumos throw -Wuninitialized false positives. > > Modified Files: > -------------- > mandoc: > read.c > > Revision Data > ------------- > Index: read.c > =================================================================== > RCS file: /home/cvs/mandoc/mandoc/read.c,v > retrieving revision 1.212 > retrieving revision 1.213 > diff -Lread.c -Lread.c -u -p -r1.212 -r1.213 > --- read.c > +++ read.c > @@ -157,7 +157,7 @@ mparse_buf_r(struct mparse *curp, struct > ln.sz = 256; > ln.buf = mandoc_malloc(ln.sz); > ln.next = NULL; > - firstln = loop = NULL; > + firstln = lastln = loop = NULL; > lnn = curp->line; > pos = 0; > inloop = 0; > > > Log Message: > ----------- > Initialize the local variable "lastln" in mparse_buf_r(). > While there is no bug, it logically makes sense given the meaning > of the variable that lastln is NULL as long as firstln is NULL. > > Michal Nowak <mnowak at startmail dot com> reported that gcc 4.4.4 > and 7.4.0 on illumos throw -Wuninitialized false positives. > > Modified Files: > -------------- > mandoc: > read.c > > Revision Data > ------------- > Index: read.c > =================================================================== > RCS file: /home/cvs/mandoc/mandoc/read.c,v > retrieving revision 1.212 > retrieving revision 1.213 > diff -Lread.c -Lread.c -u -p -r1.212 -r1.213 > --- read.c > +++ read.c > @@ -157,7 +157,7 @@ mparse_buf_r(struct mparse *curp, struct > ln.sz = 256; > ln.buf = mandoc_malloc(ln.sz); > ln.next = NULL; > - firstln = loop = NULL; > + firstln = lastln = loop = NULL; > lnn = curp->line; > pos = 0; > inloop = 0; > I think you duplicated the read.c patch here and you meant https://mandoc.bsd.lv/cgi-bin/cvsweb/term.c.diff?r1=1.280&r2=1.281 (I'll go with that one instead). Thanks again! Michal -- To unsubscribe send an email to discuss+unsubscribe@mandoc.bsd.lv