|
From: Florian K. <br...@ac...> - 2011-04-29 23:34:37
Attachments:
static-analysis-results
|
Hello.
During the past weeks I have been talking to IBM about using their
static analysis tool on valgrind. They did agree and we've been doing
some trial runs to find a configuration of the tool that produces
results with a small false positive rate.
Attached (so the mail transfer won't garble it) are some results from
a run that happened last night on VGP_x86_linux.
I've added commentary for some of the complaints. Everything else is
tool output. I'd like to run the checker regularly; once a week seems
like a good frequency. So it would be great if you could look over the
complaints and tell me which ones you consider invalid, so that they can
be suppressed in the future.
Florian
|
|
From: Robert W. <rj...@du...> - 2011-05-01 22:27:36
|
Good stuff - probably worth filing bugs for each individual report. On Fri, Apr 29, 2011 at 4:34 PM, Florian Krohm <br...@ac...> wrote: > Hello. > > During the past weeks I have been talking to IBM about using their > static analysis tool on valgrind. They did agree and we've been doing > some trial runs to find a configuration of the tool that produces > results with a small false positive rate. > Attached (so the mail transfer won't garble it) are some results from > a run that happened last night on VGP_x86_linux. > > I've added commentary for some of the complaints. Everything else is > tool output. I'd like to run the checker regularly; once a week seems > like a good frequency. So it would be great if you could look over the > complaints and tell me which ones you consider invalid, so that they can > be suppressed in the future. > > Florian > > > ------------------------------------------------------------------------------ > WhatsUp Gold - Download Free Network Management Software > The most intuitive, comprehensive, and cost-effective network > management toolset available today. Delivers lowest initial > acquisition cost and overall TCO of any competing solution. > http://p.sf.net/sfu/whatsupgold-sd > _______________________________________________ > Valgrind-developers mailing list > Val...@li... > https://lists.sourceforge.net/lists/listinfo/valgrind-developers > > |
|
From: Julian S. <js...@ac...> - 2011-05-03 08:28:42
|
There's some good stuff here, which I'm in a way disappointed that
neither gcc nor clang found. Although I guess that's not fair to
clang; I merely tried clang, not the clang static analyser.
It will take some time to fix and verify some of them. So weekly
re-runs would be good. The false error rate looks tolerable
(so far I can only see one false error) so whatever
settings you have seem to me to be OK.
Does this analyser have a name? Having to refer to it as "the IBM
checker" in email etc is a bit lame.
What's the scope of the checker? Does it operate only within
compilation units, or can it analyse across compilation units?
> Attached (so the mail transfer won't garble it) are some results from
> a run that happened last night on VGP_x86_linux.
Does that mean that the run happened on a 32-bit x86 Linux box, or that
V was configured that for the analysis run? If the latter, is it possible
to do analysis runs for other popular configurations (amd64-linux,
arm-linux) ? I presume you can't analyse the Darwin ports since
they require Darwin headers.
Some comments on the errors follow.
-----
I only looked at a few so far. I think Bart fixed the DRD ones;
Bart, is that so?
-- WARNING5 /*assignment in condition*/
>>>WARNING5_disInstr_ARM_WRK_14bf2881505
"VEX/priv/guest_arm_toIR.c", line 14013: operator = in the boolean expression
should possibly be ==
Urr. That might have to do with why Callgrind gets confused about
calls/returns when running ARM code.
-----
-- MISTAKE5 /*constant condition*/
>>>MISTAKE5_iselIntExpr_R_wrk_1f6612751505
"VEX/priv/host_amd64_isel.c", line 866: The expression `(int)Ity_I16' is true
whenever evaluated
You Typed: vassert(ty == Ity_I32 || Ity_I16 || Ity_I8)
This is a stupid copy-n-paste gone wrong from the x86 equivalent.
Because Ity_I16 and Ity_I8 are nonzero, this is equivalent to
vassert(True). Fixed in r2141.
-----
-- MISTAKE5 /*constant condition*/
>>>MISTAKE5_mkLazyN_ab34cd99c4fb48ec
"memcheck/mc_translate.c", line 1588: The expression `mergeTy64' is true
whenever evaluated
COMMENT: The checker assumes (by default) that a variable of enumerated type
can only assume the value of any of the enumerators defined for its
type.
The enumerators for IRType all have values > 0; Hence the complaint.
I believe the fix here is to declare mergeTy64 as Bool.
Trivial to fix (as you say); what is difficult is to figure out / verify
if the fix affects the performance or accuracy of Memcheck on floating
point code, since afaics the fix will change the instrumentation code
Memcheck generates.
-----
-- ERROR9 /*passing null object*/
>>>ERROR9_handle_maybe_load_notifier_e2c77f1b1505
"coregrind/m_redir.c", line 1190: passing NULL to argument 1 of
`vgPlain_strncmp'
ONE POSSIBLE PATH LEADING TO THE ERROR:
"coregrind/m_redir.c", line 1180: conjunct is false (used as evidence that
error is possible)
"coregrind/m_redir.c", line 1180: the if-condition is false
"coregrind/m_redir.c", line 1190: calling `vgPlain_strncmp'
I didn't understand this. Where does it think the NULL comes from?
I don't see it.
-----
-- ERROR2 /*operating on NULL*/
>>>ERROR2_alloc_client_block_6975911e2b60e
"memcheck/mc_main.c", line 4955: invalid operation involving NULL pointer
ONE POSSIBLE PATH LEADING TO THE ERROR:
"memcheck/mc_main.c", line 4937: assuming `i == cgb_used - 1'
"memcheck/mc_main.c", line 4937: loop entry condition is true
"memcheck/mc_main.c", line 4939: the if-condition is false
"memcheck/mc_main.c", line 4937: going around the loop again
"memcheck/mc_main.c", line 4937: loop entry condition is false, therefore
exiting from the loop started on line 4937
"memcheck/mc_main.c", line 4944: assuming `cgb_used == cgb_size'
"memcheck/mc_main.c", line 4944: the if-condition is false
"memcheck/mc_main.c", line 4951: the ?-condition is true (used as evidence
that error is possible)
"memcheck/mc_main.c", line 4954: loop entry condition is true
"memcheck/mc_main.c", line 4955: using operation `[]' to dereference NULL
pointer `cgbs'
VALUES AT THE END OF THE PATH:
cgbs = 0
i = 0
This seems bogus to me, because
"memcheck/mc_main.c", line 4939: the if-condition is false
means that this ?-condition can't be true
"memcheck/mc_main.c", line 4951: the ?-condition is true (used as evidence
that error is possible)
If cgbs == NULL at line 4951 then we would have segfaulted at 4939.
-----
-- ERROR9 /*passing null object*/
>>>ERROR9_setup_client_stack_39174a691505
"coregrind/m_initimg/initimg-linux.c", line 460: passing NULL to argument 1 of
`vgPlain_strlen'
ONE POSSIBLE PATH LEADING TO THE ERROR:
"coregrind/m_initimg/initimg-linux.c", line 434: the if-condition is false
(used as evidence that error is possible)
The extensive path it mentions doesn't seem helpful. It is clear however
that the argument to VG_(strlen) -- that is, VG_(args_the_exename)
is passed without a NULL check, whereas above in the same function, it is
NULL checked before use.
-----
-- ERROR5 /*dereferencing NULL*/
>>>ERROR5_vgPlain_env_unsetenv_dfc79f795979a8
"coregrind/m_libcproc.c", line 104: dereferencing NULL pointer `to'
Yeah, I guess this could fault if the supplied 'env' is NULL.
We should assert against it.
-----
-- ERROR5 /*dereferencing NULL*/
>>>ERROR5_vgPlain_env_clone_5c0c0e06777f13d
"coregrind/m_libcproc.c", line 319: dereferencing NULL pointer `oldenvp'
Probably could fault if the supplied 'oldenv' is NULL.
|
|
From: Bart V. A. <bva...@ac...> - 2011-05-03 10:38:12
|
On Tue, May 3, 2011 at 10:26 AM, Julian Seward <js...@ac...> wrote: > > There's some good stuff here, which I'm in a way disappointed that > neither gcc nor clang found. Although I guess that's not fair to > clang; I merely tried clang, not the clang static analyser. > It will take some time to fix and verify some of them. So weekly > re-runs would be good. The false error rate looks tolerable > (so far I can only see one false error) so whatever > settings you have seem to me to be OK. > > Does this analyser have a name? Having to refer to it as "the IBM > checker" in email etc is a bit lame. > > What's the scope of the checker? Does it operate only within > compilation units, or can it analyse across compilation units? > > > > Attached (so the mail transfer won't garble it) are some results from > > a run that happened last night on VGP_x86_linux. > > Does that mean that the run happened on a 32-bit x86 Linux box, or that > V was configured that for the analysis run? If the latter, is it possible > to do analysis runs for other popular configurations (amd64-linux, > arm-linux) ? I presume you can't analyse the Darwin ports since > they require Darwin headers. > > Some comments on the errors follow. > > ----- > > I only looked at a few so far. I think Bart fixed the DRD ones; > Bart, is that so? > Yes, that's correct. Bart. |
|
From: Florian K. <br...@ac...> - 2011-05-03 14:05:59
|
On 05/03/2011 04:26 AM, Julian Seward wrote: > > Does this analyser have a name? Having to refer to it as "the IBM > checker" in email etc is a bit lame. > The term "IBM checker" used to be a pun on the "Stanford checker" from long time ago which now is Coverity. It is a bit lame I agree. The internal name is BEAM which stands for Bugs, Errors, and Mistakes. https://researcher.ibm.com/researcher/view_project.php?id=1628 Perhaps we can refer to it as "IBM checker BEAM" or "IBM's BEAM checker". It's probably fair to give IBM some credit for letting us use the tool. > What's the scope of the checker? Does it operate only within > compilation units, or can it analyse across compilation units? > Analysis is across compilation units. > >> Attached (so the mail transfer won't garble it) are some results from >> a run that happened last night on VGP_x86_linux. > > Does that mean that the run happened on a 32-bit x86 Linux box, or that > V was configured that for the analysis run? Valgrind was configured such that VGP_x86_linux is defined. The analysis was done assuming an ILP32 target which is consistent. > If the latter, is it possible > to do analysis runs for other popular configurations (amd64-linux, > arm-linux) ? I presume you can't analyse the Darwin ports since > they require Darwin headers. > We'll have weekly runs s390x for sure :). I see whether I can arrange for x86 or amd64. The exposure of missing some defects is probably not that large. BEAM did analyse all those files that were compiled during a build. ARM is definitely not an option. If Maynard is interested in setting up runs on ppc I'd be happy to point him to the people to talk to. > Some comments on the errors follow. > [snip] > > > -- MISTAKE5 /*constant condition*/ >>>> MISTAKE5_mkLazyN_ab34cd99c4fb48ec > "memcheck/mc_translate.c", line 1588: The expression `mergeTy64' is true > whenever evaluated > > COMMENT: The checker assumes (by default) that a variable of enumerated type > can only assume the value of any of the enumerators defined for its > type. > The enumerators for IRType all have values > 0; Hence the complaint. > I believe the fix here is to declare mergeTy64 as Bool. > > > Trivial to fix (as you say); what is difficult is to figure out / verify > if the fix affects the performance or accuracy of Memcheck on floating > point code, since afaics the fix will change the instrumentation code > Memcheck generates. > I do not think memcheck instrumentation will be affected. There are two variables: mergeTy and mergeTy64. The former affects memcheck because it is passed to mkPCastTo. The latter is the one whose type should be changed to Bool. And that won't affect memcheck as far as I can see. > ----- > > -- ERROR9 /*passing null object*/ >>>> ERROR9_handle_maybe_load_notifier_e2c77f1b1505 > "coregrind/m_redir.c", line 1190: passing NULL to argument 1 of > `vgPlain_strncmp' > ONE POSSIBLE PATH LEADING TO THE ERROR: > "coregrind/m_redir.c", line 1180: conjunct is false (used as evidence that > error is possible) > "coregrind/m_redir.c", line 1180: the if-condition is false > "coregrind/m_redir.c", line 1190: calling `vgPlain_strncmp' > > I didn't understand this. Where does it think the NULL comes from? > I don't see it. > Line 1180: if (symbol && symbol[0] == '_' In other words: if (symbol != NULL && symbol[0] == '_' So you're considering the case that symbol could be NULL. Then it's passed to vgPlain_strncmp unguarded. > ----- > > -- ERROR2 /*operating on NULL*/ >>>> ERROR2_alloc_client_block_6975911e2b60e > "memcheck/mc_main.c", line 4955: invalid operation involving NULL pointer > ONE POSSIBLE PATH LEADING TO THE ERROR: > "memcheck/mc_main.c", line 4937: assuming `i == cgb_used - 1' > "memcheck/mc_main.c", line 4937: loop entry condition is true > "memcheck/mc_main.c", line 4939: the if-condition is false > "memcheck/mc_main.c", line 4937: going around the loop again > "memcheck/mc_main.c", line 4937: loop entry condition is false, therefore > exiting from the loop started on line 4937 > "memcheck/mc_main.c", line 4944: assuming `cgb_used == cgb_size' > "memcheck/mc_main.c", line 4944: the if-condition is false > "memcheck/mc_main.c", line 4951: the ?-condition is true (used as evidence > that error is possible) > "memcheck/mc_main.c", line 4954: loop entry condition is true > "memcheck/mc_main.c", line 4955: using operation `[]' to dereference NULL > pointer `cgbs' > > VALUES AT THE END OF THE PATH: > cgbs = 0 > i = 0 > > > This seems bogus to me, because > "memcheck/mc_main.c", line 4939: the if-condition is false > means that this ?-condition can't be true > "memcheck/mc_main.c", line 4951: the ?-condition is true (used as evidence > that error is possible) > > If cgbs == NULL at line 4951 then we would have segfaulted at 4939. > I think the reason why the report is for line 4955 and not for 4939 is that on line 4939 there is no evidence that cgbs could be NULL. That is only established on line 4951. > > -- ERROR5 /*dereferencing NULL*/ >>>> ERROR5_vgPlain_env_unsetenv_dfc79f795979a8 > "coregrind/m_libcproc.c", line 104: dereferencing NULL pointer `to' > > Yeah, I guess this could fault if the supplied 'env' is NULL. > We should assert against it. Yep, I agree, that would be good. Florian |
|
From: Josef W. <Jos...@gm...> - 2011-05-11 17:57:00
|
On Tuesday 03 May 2011, Julian Seward wrote: > -- WARNING5 /*assignment in condition*/ > >>>WARNING5_disInstr_ARM_WRK_14bf2881505 > "VEX/priv/guest_arm_toIR.c", line 14013: operator = in the boolean expression > should possibly be == > > Urr. That might have to do with why Callgrind gets confused about > calls/returns when running ARM code. Wishful thinking ;-) For sure, there is more to do here. Josef |
|
From: Julian S. <js...@ac...> - 2011-05-04 10:05:46
|
> Attached (so the mail transfer won't garble it) are some results from > a run that happened last night on VGP_x86_linux. I made the following commits, to fix complaints in memcheck, helgrind, coregrind and VEX. Nick, Josef, can you pls look at the Cachegrind and Callgrind complaints when you have time? The majority of the ones I looked at seem real, so I think these are worth chasing. Florian, can you maybe do a re-run on Friday so we can see what remains to be fixed? Thanks. J -- WARNING5 /*assignment in condition*/ >>>WARNING5_disInstr_ARM_WRK_14bf2881505 Fixed, r2142; a real (and stupid) bug. -- ERROR2 /*operating on NULL*/ >>>ERROR2_vgDrd_cond_post_wait_6552c7f0ff59258 "drd/drd_cond.c", line 304: invalid operation involving NULL pointer Fixed (Bart), r11719. -- MISTAKE5 /*constant condition*/ >>>MISTAKE5_vgDrd_semaphore_post_wait_62d0d5eab874a3f "drd/drd_semaphore.c", line 408: The expression `sg' is true whenever evaluated Fixed (Bart), r11719. -- ERROR13 /*format mismatch*/ >>>ERROR13_scalarts_limitations_fail_NORETURN_27d9b0199b79cb13 "helgrind/libhb_core.c", line 1604: vgPlain_umsg argument `ThrID_MAX_VALID - 1024' of type `int' cannot have the underlined specification Fixed, r11725. -- ERROR9 /*passing null object*/ >>>ERROR9_handle_maybe_load_notifier_e2c77f1b1505 "coregrind/m_redir.c", line 1190: passing NULL to argument 1 of `vgPlain_strncmp' Fixed, r11722. -- MISTAKE5 /*constant condition*/ >>>MISTAKE5_iselIntExpr_R_wrk_1f6612751505 "VEX/priv/host_amd64_isel.c", line 866: The expression `(int)Ity_I16' is true whenever evaluated Fixed, r2141, stupid copy-n-paste nonsense. -- ERROR39 /*shift undefined*/ >>>ERROR39_s390_emit_LGDRw_3a81323b1505 -- ERROR39 /*shift undefined*/ >>>ERROR39_s390_emit_LDGRw_a068b9a51505 I assume Florian has patches for these pending? -- ERROR1 /*uninitialized*/ >>>ERROR1_doHelperCall_b054d3a0b2ee0073 "VEX/priv/host_ppc_isel.c", line 858: uninitialized `cc.flag' No error in the code. "cc" is a two part struct; when one part is Pct_ALWAYS, the other part isn't used, but BEAM doesn't see that. I added code to fix placate it anyway. r2144 -- MISTAKE5 /*constant condition*/ >>>MISTAKE5_mkLazyN_ab34cd99c4fb48ec "memcheck/mc_translate.c", line 1588: The expression `mergeTy64' is true whenever evaluated Fixed (real bug), r11726. -- ERROR1 /*uninitialized*/ >>>ERROR1_vgPlain_get_data_description_2f5a8e26b88ae46 "coregrind/m_debuginfo/debuginfo.c", line 3155: uninitialized `tid' No error in code. Certain arg values (frameNo == -1) cause 'tid' to not be used, but BEAM didn't seem to spot that. Added code to keep it happy anyway, r11723. -- ERROR9 /*passing null object*/ >>>ERROR9_setup_client_stack_39174a691505 "coregrind/m_initimg/initimg-linux.c", line 460: passing NULL to argument 1 of `vgPlain_strlen' Fixed, r11724. -- ERROR5 /*dereferencing NULL*/ >>>ERROR5_vgPlain_env_unsetenv_dfc79f795979a8 "coregrind/m_libcproc.c", line 104: dereferencing NULL pointer `to' Added assertion, r11721. -- ERROR5 /*dereferencing NULL*/ >>>ERROR5_vgPlain_env_clone_5c0c0e06777f13d "coregrind/m_libcproc.c", line 319: dereferencing NULL pointer `oldenvp' Added assertion, r11721. |
|
From: Florian K. <br...@ac...> - 2011-05-04 12:26:12
|
On 05/04/2011 06:03 AM, Julian Seward wrote: > > Florian, can you maybe do a re-run on Friday so we can see what remains > to be fixed? Thanks. > Yes, will do. > -- ERROR39 /*shift undefined*/ >>>ERROR39_s390_emit_LGDRw_3a81323b1505 > -- ERROR39 /*shift undefined*/ >>>ERROR39_s390_emit_LDGRw_a068b9a51505 > > I assume Florian has patches for these pending? > Yes: https://bugs.kde.org/show_bug.cgi?id=272067 Please apply at your convenience. Florian |