discuss@mandoc.bsd.lv
 help / color / Atom feed
* error: 'XXX' may be used uninitialized in this function
@ 2019-06-03 15:28 Michal Nowak
  2019-06-03 20:37 ` Ingo Schwarze
  0 siblings, 1 reply; 3+ messages in thread
From: Michal Nowak @ 2019-06-03 15:28 UTC (permalink / raw)
  To: discuss

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: error: 'XXX' may be used uninitialized in this function
  2019-06-03 15:28 error: 'XXX' may be used uninitialized in this function Michal Nowak
@ 2019-06-03 20:37 ` Ingo Schwarze
  2019-06-04  4:53   ` Michal Nowak
  0 siblings, 1 reply; 3+ messages in thread
From: Ingo Schwarze @ 2019-06-03 20:37 UTC (permalink / raw)
  To: Michal Nowak; +Cc: discuss

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: error: 'XXX' may be used uninitialized in this function
  2019-06-03 20:37 ` Ingo Schwarze
@ 2019-06-04  4:53   ` Michal Nowak
  0 siblings, 0 replies; 3+ messages in thread
From: Michal Nowak @ 2019-06-04  4:53 UTC (permalink / raw)
  To: Ingo Schwarze; +Cc: discuss

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-03 15:28 error: 'XXX' may be used uninitialized in this function Michal Nowak
2019-06-03 20:37 ` Ingo Schwarze
2019-06-04  4:53   ` Michal Nowak

discuss@mandoc.bsd.lv

Archives are clonable: git clone --mirror http://inbox.vuxu.org/mandoc-discuss

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://inbox.vuxu.org/vuxu.archive.mandoc.discuss


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git