Thread: [perfmon2] libpfm-4.3.0 released
Status: Beta
Brought to you by:
seranian
From: stephane e. <er...@go...> - 2012-08-28 10:30:03
|
Hello, I have finally released libpfm version 4.3.0. As expected, it includes many updates, improvements and bug fixes. Many thanks to all the people who have contributed to this release. Highlights: - ARM Cortex A15 support (Thanks to Will Deacon for his contribution) - updated Intel Sandy Bridge core PMU events - Intel Sandy Bridge desktop (model 42) uncore PMU support - Intel Ivy Bridge core PMU support - full perf_events generic event support (e.g., cycles, instructions) - updated perf_examples - enabled Intel Nehalem/Westmere uncore PMU support - AMD LLano processor support (Fam 12h) (Thanks to Vince Weaver) - AMD Turion processor support (Fam 11h) (Thanks to Vince Weaver) - Intel Atom Cedarview processor support - Win32 compilation support - perf_events excl attribute - many bug fixes You can download the tarball from: http://sourceforge.net/projects/perfmon2/files/libpfm4/libpfm-4.3.0.tar.gz/download Hopefully, I will be able to releases 4.4.0 much sooner next time. |
From: William C. <wc...@re...> - 2012-08-28 14:45:29
|
On 08/28/2012 06:29 AM, stephane eranian wrote: > Hello, > > I have finally released libpfm version 4.3.0. > > As expected, it includes many updates, improvements and bug fixes. > Many thanks to all the people who have contributed to this release. > > Highlights: > > - ARM Cortex A15 support (Thanks to Will Deacon for his contribution) > - updated Intel Sandy Bridge core PMU events > - Intel Sandy Bridge desktop (model 42) uncore PMU support > - Intel Ivy Bridge core PMU support > - full perf_events generic event support (e.g., cycles, instructions) > - updated perf_examples > - enabled Intel Nehalem/Westmere uncore PMU support > - AMD LLano processor support (Fam 12h) (Thanks to Vince Weaver) > - AMD Turion processor support (Fam 11h) (Thanks to Vince Weaver) > - Intel Atom Cedarview processor support > - Win32 compilation support > - perf_events excl attribute > - many bug fixes > > You can download the tarball from: > http://sourceforge.net/projects/perfmon2/files/libpfm4/libpfm-4.3.0.tar.gz/download > > Hopefully, I will be able to releases 4.4.0 much sooner next time. Hi Stephane, Thanks so much for the fresh libpfm-4.3.0 release. I just built a version for fedora rawhide at: http://koji.fedoraproject.org/koji/buildinfo?buildID=350950 I ran ./libpfm-4.3.0/tests/validate on a fedora 17 Intel Ivy bridge and AMD family 10H machine. The tests all listed as pass with the exception of the following for Intel Ivy Bridge: checking perf (120 events): pmu: perf event80: sunrpc :: numasks too big (<8) Failed And the following for AMD family 10H: checking perf (119 events): pmu: perf event85: sunrpc :: numasks too big (<8) Failed Would it be possible to do the ldconf separately from the "make install"? The rpm packages are built as normal users with a staged install and the ldconf is done as a post install operation. The ldconf in the makefile needed to be disabled for the libpfm rpm to be built. -Will |
From: stephane e. <er...@go...> - 2012-08-28 16:03:13
|
On Tue, Aug 28, 2012 at 4:45 PM, William Cohen <wc...@re...> wrote: > On 08/28/2012 06:29 AM, stephane eranian wrote: >> Hello, >> >> I have finally released libpfm version 4.3.0. >> >> As expected, it includes many updates, improvements and bug fixes. >> Many thanks to all the people who have contributed to this release. >> >> Highlights: >> >> - ARM Cortex A15 support (Thanks to Will Deacon for his contribution) >> - updated Intel Sandy Bridge core PMU events >> - Intel Sandy Bridge desktop (model 42) uncore PMU support >> - Intel Ivy Bridge core PMU support >> - full perf_events generic event support (e.g., cycles, instructions) >> - updated perf_examples >> - enabled Intel Nehalem/Westmere uncore PMU support >> - AMD LLano processor support (Fam 12h) (Thanks to Vince Weaver) >> - AMD Turion processor support (Fam 11h) (Thanks to Vince Weaver) >> - Intel Atom Cedarview processor support >> - Win32 compilation support >> - perf_events excl attribute >> - many bug fixes >> >> You can download the tarball from: >> http://sourceforge.net/projects/perfmon2/files/libpfm4/libpfm-4.3.0.tar.gz/download >> >> Hopefully, I will be able to releases 4.4.0 much sooner next time. > > Hi Stephane, > > Thanks so much for the fresh libpfm-4.3.0 release. I just built a version for fedora rawhide at: > > http://koji.fedoraproject.org/koji/buildinfo?buildID=350950 > > I ran ./libpfm-4.3.0/tests/validate on a fedora 17 Intel Ivy bridge and AMD family 10H machine. The tests all listed as pass with the exception of the following for Intel Ivy Bridge: > > checking perf (120 events): pmu: perf event80: sunrpc :: numasks too big (<8) > Failed > > And the following for AMD family 10H: > > checking perf (119 events): pmu: perf event85: sunrpc :: numasks too big (<8) > Failed > > Ok, will look at those two. > > Would it be possible to do the ldconf separately from the "make install"? The rpm packages are built as normal users with a staged install and the ldconf is done as a post install operation. The ldconf in the makefile needed to be disabled for the libpfm rpm to be built. > I just updated the libpfm.spec today to convert ldconfig to a nop (LDCONFIG=true), so it should not perturb the generation of the RPM. At least, it did not for me when I tried on Fedora 16. |
From: William C. <wc...@re...> - 2012-08-28 19:17:18
Attachments:
libpfm-4.3.0-1.fc19.err
|
On 08/28/2012 12:03 PM, stephane eranian wrote: > On Tue, Aug 28, 2012 at 4:45 PM, William Cohen <wc...@re...> wrote: >> >> Would it be possible to do the ldconf separately from the "make install"? The rpm packages are built as normal users with a staged install and the ldconf is done as a post install operation. The ldconf in the makefile needed to be disabled for the libpfm rpm to be built. >> > I just updated the libpfm.spec today to convert ldconfig to a nop > (LDCONFIG=true), so > it should not perturb the generation of the RPM. At least, it did not > for me when I tried > on Fedora 16. > Hi Stephane, Thanks for the libpfm.spec. The fedora rawhide libpfm-4.3.0 doesn't have any patches in it now: http://koji.fedoraproject.org/koji/taskinfo?taskID=4432221 I ran libpfm through coverity to see whether there were any serious problems with it. Attached is the output of coverity. All errors are in the generated swig code and the examples. Probably can't do much about the swig related error because there are all in generated code. The examples could be cleaned up a little, but those are not packaged in the libpfm rpms, so those are not a great problem. In perf_setup_list_events() in perf_util.c why not make the for loop replace the ',' with '\0' the same structure as the earlier while loop counting the args? That would make the code a bit easier to read and eliminate one of the coverity errors. . -Will |
From: stephane e. <er...@go...> - 2012-08-29 10:29:00
|
On Tue, Aug 28, 2012 at 4:45 PM, William Cohen <wc...@re...> wrote: > On 08/28/2012 06:29 AM, stephane eranian wrote: >> Hello, >> >> I have finally released libpfm version 4.3.0. >> >> As expected, it includes many updates, improvements and bug fixes. >> Many thanks to all the people who have contributed to this release. >> >> Highlights: >> >> - ARM Cortex A15 support (Thanks to Will Deacon for his contribution) >> - updated Intel Sandy Bridge core PMU events >> - Intel Sandy Bridge desktop (model 42) uncore PMU support >> - Intel Ivy Bridge core PMU support >> - full perf_events generic event support (e.g., cycles, instructions) >> - updated perf_examples >> - enabled Intel Nehalem/Westmere uncore PMU support >> - AMD LLano processor support (Fam 12h) (Thanks to Vince Weaver) >> - AMD Turion processor support (Fam 11h) (Thanks to Vince Weaver) >> - Intel Atom Cedarview processor support >> - Win32 compilation support >> - perf_events excl attribute >> - many bug fixes >> >> You can download the tarball from: >> http://sourceforge.net/projects/perfmon2/files/libpfm4/libpfm-4.3.0.tar.gz/download >> >> Hopefully, I will be able to releases 4.4.0 much sooner next time. > > Hi Stephane, > > Thanks so much for the fresh libpfm-4.3.0 release. I just built a version for fedora rawhide at: > > http://koji.fedoraproject.org/koji/buildinfo?buildID=350950 > > I ran ./libpfm-4.3.0/tests/validate on a fedora 17 Intel Ivy bridge and AMD family 10H machine. The tests all listed as pass with the exception of the following for Intel Ivy Bridge: > > checking perf (120 events): pmu: perf event80: sunrpc :: numasks too big (<8) > Failed > > And the following for AMD family 10H: > > checking perf (119 events): pmu: perf event85: sunrpc :: numasks too big (<8) > Failed > Weird, because I don't see that running on my Ubuntu distro. The sunrpc word looks suspicous to me. I don't think there is an event or umask with that name. > > > Would it be possible to do the ldconf separately from the "make install"? The rpm packages are built as normal users with a staged install and the ldconf is done as a post install operation. The ldconf in the makefile needed to be disabled for the libpfm rpm to be built. > > -Will |
From: William C. <wc...@re...> - 2012-08-29 15:02:32
|
On 08/29/2012 06:28 AM, stephane eranian wrote: > On Tue, Aug 28, 2012 at 4:45 PM, William Cohen <wc...@re...> wrote: >> Hi Stephane, >> >> Thanks so much for the fresh libpfm-4.3.0 release. I just built a version for fedora rawhide at: >> >> http://koji.fedoraproject.org/koji/buildinfo?buildID=350950 >> >> I ran ./libpfm-4.3.0/tests/validate on a fedora 17 Intel Ivy bridge and AMD family 10H machine. The tests all listed as pass with the exception of the following for Intel Ivy Bridge: >> >> checking perf (120 events): pmu: perf event80: sunrpc :: numasks too big (<8) >> Failed >> >> And the following for AMD family 10H: >> >> checking perf (119 events): pmu: perf event85: sunrpc :: numasks too big (<8) >> Failed >> > Weird, because I don't see that running on my Ubuntu distro. > The sunrpc word looks suspicous to me. I don't think there is an event > or umask with that name. Hi Stephane, libpfm is using the enumeration for perf to find out which events are avilable on the machine. It looks like the first tracepoint event listed from "perf list" is" sunrpc:rpc_call_status [Tracepoint event] Are the fields for sunrpc correct? (gdb) print perf_pe[80] $10 = {name = 0x6aa4a0 "sunrpc", desc = 0x41a519 "tracepoint", equiv = 0x0, id = 0, modmsk = 0, type = 2, numasks = 8, ngrp = 1, umask_ovfl_idx = 18446744073709551615, umasks = {{ uname = 0x6aa4c0 "rpc_call_status", udesc = 0x6aa4c0 "rpc_call_status", uid = 1037, uflags = 0, grpid = 0}, {uname = 0x6aa4e0 "rpc_bind_status", udesc = 0x6aa4e0 "rpc_bind_status", uid = 1036, uflags = 0, grpid = 0}, { uname = 0x6aa500 "rpc_connect_status", udesc = 0x6aa500 "rpc_connect_status", uid = 1035, uflags = 0, grpid = 0}, {uname = 0x6aa520 "rpc_task_begin", udesc = 0x6aa520 "rpc_task_begin", uid = 1034, uflags = 0, grpid = 0}, { uname = 0x6aa540 "rpc_task_run_action", udesc = 0x6aa540 "rpc_task_run_action", uid = 1033, uflags = 0, grpid = 0}, {uname = 0x6aa560 "rpc_task_complete", udesc = 0x6aa560 "rpc_task_complete", uid = 1032, uflags = 0, grpid = 0}, {uname = 0x6aa580 "rpc_task_sleep", udesc = 0x6aa580 "rpc_task_sleep", uid = 1031, uflags = 0, grpid = 0}, { uname = 0x6aa5a0 "rpc_task_wakeup", udesc = 0x6aa5a0 "rpc_task_wakeup", uid = 1030, uflags = 0, grpid = 0}}} It looks like perf_pe[80].umask_ovfl_idx should be 0 like the later entries. -Will |
From: stephane e. <er...@go...> - 2012-08-29 15:32:23
|
On Wed, Aug 29, 2012 at 5:02 PM, William Cohen <wc...@re...> wrote: > On 08/29/2012 06:28 AM, stephane eranian wrote: >> On Tue, Aug 28, 2012 at 4:45 PM, William Cohen <wc...@re...> wrote: > >>> Hi Stephane, >>> >>> Thanks so much for the fresh libpfm-4.3.0 release. I just built a version for fedora rawhide at: >>> >>> http://koji.fedoraproject.org/koji/buildinfo?buildID=350950 >>> >>> I ran ./libpfm-4.3.0/tests/validate on a fedora 17 Intel Ivy bridge and AMD family 10H machine. The tests all listed as pass with the exception of the following for Intel Ivy Bridge: >>> >>> checking perf (120 events): pmu: perf event80: sunrpc :: numasks too big (<8) >>> Failed >>> >>> And the following for AMD family 10H: >>> >>> checking perf (119 events): pmu: perf event85: sunrpc :: numasks too big (<8) >>> Failed >>> >> Weird, because I don't see that running on my Ubuntu distro. >> The sunrpc word looks suspicous to me. I don't think there is an event >> or umask with that name. > > Hi Stephane, > > libpfm is using the enumeration for perf to find out which events are avilable on the machine. It looks like the first tracepoint event listed from "perf list" is" > > sunrpc:rpc_call_status [Tracepoint event] > > Are the fields for sunrpc correct? > > (gdb) print perf_pe[80] > $10 = {name = 0x6aa4a0 "sunrpc", desc = 0x41a519 "tracepoint", equiv = 0x0, > id = 0, modmsk = 0, type = 2, numasks = 8, ngrp = 1, > umask_ovfl_idx = 18446744073709551615, umasks = {{ > uname = 0x6aa4c0 "rpc_call_status", udesc = 0x6aa4c0 "rpc_call_status", > uid = 1037, uflags = 0, grpid = 0}, {uname = 0x6aa4e0 "rpc_bind_status", > udesc = 0x6aa4e0 "rpc_bind_status", uid = 1036, uflags = 0, grpid = 0}, { > uname = 0x6aa500 "rpc_connect_status", > udesc = 0x6aa500 "rpc_connect_status", uid = 1035, uflags = 0, > grpid = 0}, {uname = 0x6aa520 "rpc_task_begin", > udesc = 0x6aa520 "rpc_task_begin", uid = 1034, uflags = 0, grpid = 0}, { > uname = 0x6aa540 "rpc_task_run_action", > udesc = 0x6aa540 "rpc_task_run_action", uid = 1033, uflags = 0, > grpid = 0}, {uname = 0x6aa560 "rpc_task_complete", > udesc = 0x6aa560 "rpc_task_complete", uid = 1032, uflags = 0, > grpid = 0}, {uname = 0x6aa580 "rpc_task_sleep", > udesc = 0x6aa580 "rpc_task_sleep", uid = 1031, uflags = 0, grpid = 0}, { > uname = 0x6aa5a0 "rpc_task_wakeup", udesc = 0x6aa5a0 "rpc_task_wakeup", > uid = 1030, uflags = 0, grpid = 0}}} > > It looks like perf_pe[80].umask_ovfl_idx should be 0 like the later entries. > Ok, I know what I did not catch that. You need to run as root to be able to parse /sys/kernel/debug AND you need to have the sunrpc code builtin or module loaded. I had neither of those two. Let me fix this. |
From: stephane e. <er...@go...> - 2012-08-29 16:23:24
|
Problem solved. Test was wrong. Was using >= instead of >. It hit this event only because it had exactly 8 umasks! Thanks for reporting and tracking down the problem. Please pull again. On Wed, Aug 29, 2012 at 5:32 PM, stephane eranian <er...@go...> wrote: > On Wed, Aug 29, 2012 at 5:02 PM, William Cohen <wc...@re...> wrote: >> On 08/29/2012 06:28 AM, stephane eranian wrote: >>> On Tue, Aug 28, 2012 at 4:45 PM, William Cohen <wc...@re...> wrote: >> >>>> Hi Stephane, >>>> >>>> Thanks so much for the fresh libpfm-4.3.0 release. I just built a version for fedora rawhide at: >>>> >>>> http://koji.fedoraproject.org/koji/buildinfo?buildID=350950 >>>> >>>> I ran ./libpfm-4.3.0/tests/validate on a fedora 17 Intel Ivy bridge and AMD family 10H machine. The tests all listed as pass with the exception of the following for Intel Ivy Bridge: >>>> >>>> checking perf (120 events): pmu: perf event80: sunrpc :: numasks too big (<8) >>>> Failed >>>> >>>> And the following for AMD family 10H: >>>> >>>> checking perf (119 events): pmu: perf event85: sunrpc :: numasks too big (<8) >>>> Failed >>>> >>> Weird, because I don't see that running on my Ubuntu distro. >>> The sunrpc word looks suspicous to me. I don't think there is an event >>> or umask with that name. >> >> Hi Stephane, >> >> libpfm is using the enumeration for perf to find out which events are avilable on the machine. It looks like the first tracepoint event listed from "perf list" is" >> >> sunrpc:rpc_call_status [Tracepoint event] >> >> Are the fields for sunrpc correct? >> >> (gdb) print perf_pe[80] >> $10 = {name = 0x6aa4a0 "sunrpc", desc = 0x41a519 "tracepoint", equiv = 0x0, >> id = 0, modmsk = 0, type = 2, numasks = 8, ngrp = 1, >> umask_ovfl_idx = 18446744073709551615, umasks = {{ >> uname = 0x6aa4c0 "rpc_call_status", udesc = 0x6aa4c0 "rpc_call_status", >> uid = 1037, uflags = 0, grpid = 0}, {uname = 0x6aa4e0 "rpc_bind_status", >> udesc = 0x6aa4e0 "rpc_bind_status", uid = 1036, uflags = 0, grpid = 0}, { >> uname = 0x6aa500 "rpc_connect_status", >> udesc = 0x6aa500 "rpc_connect_status", uid = 1035, uflags = 0, >> grpid = 0}, {uname = 0x6aa520 "rpc_task_begin", >> udesc = 0x6aa520 "rpc_task_begin", uid = 1034, uflags = 0, grpid = 0}, { >> uname = 0x6aa540 "rpc_task_run_action", >> udesc = 0x6aa540 "rpc_task_run_action", uid = 1033, uflags = 0, >> grpid = 0}, {uname = 0x6aa560 "rpc_task_complete", >> udesc = 0x6aa560 "rpc_task_complete", uid = 1032, uflags = 0, >> grpid = 0}, {uname = 0x6aa580 "rpc_task_sleep", >> udesc = 0x6aa580 "rpc_task_sleep", uid = 1031, uflags = 0, grpid = 0}, { >> uname = 0x6aa5a0 "rpc_task_wakeup", udesc = 0x6aa5a0 "rpc_task_wakeup", >> uid = 1030, uflags = 0, grpid = 0}}} >> >> It looks like perf_pe[80].umask_ovfl_idx should be 0 like the later entries. >> > Ok, I know what I did not catch that. > You need to run as root to be able to parse /sys/kernel/debug > AND you need to have the sunrpc code builtin or module loaded. > I had neither of those two. Let me fix this. |
From: stephane e. <er...@go...> - 2012-09-03 14:23:47
|
Will, I have fixed most of the warnings reported by your tool. Please pull and try again. On Tue, Aug 28, 2012 at 9:17 PM, William Cohen <wc...@re...> wrote: > On 08/28/2012 12:03 PM, stephane eranian wrote: >> On Tue, Aug 28, 2012 at 4:45 PM, William Cohen <wc...@re...> wrote: > >>> >>> Would it be possible to do the ldconf separately from the "make install"? The rpm packages are built as normal users with a staged install and the ldconf is done as a post install operation. The ldconf in the makefile needed to be disabled for the libpfm rpm to be built. >>> >> I just updated the libpfm.spec today to convert ldconfig to a nop >> (LDCONFIG=true), so >> it should not perturb the generation of the RPM. At least, it did not >> for me when I tried >> on Fedora 16. >> > > Hi Stephane, > > > Thanks for the libpfm.spec. The fedora rawhide libpfm-4.3.0 doesn't have any patches in it now: > > http://koji.fedoraproject.org/koji/taskinfo?taskID=4432221 > > I ran libpfm through coverity to see whether there were any serious problems with it. Attached is the output of coverity. All errors are in the generated swig code and the examples. Probably can't do much about the swig related error because there are all in generated code. The examples could be cleaned up a little, but those are not packaged in the libpfm rpms, so those are not a great problem. > > In perf_setup_list_events() in perf_util.c why not make the for loop replace the ',' with '\0' the same structure as the earlier while loop counting the args? That would make the code a bit easier to read and eliminate one of the coverity errors. > . > -Will |
From: William C. <wc...@re...> - 2012-09-04 14:25:51
Attachments:
libpfm-4.3.0-2.fc17cov.err
|
On 09/03/2012 10:23 AM, stephane eranian wrote: > Will, > > I have fixed most of the warnings reported by your tool. > Please pull and try again. > > > On Tue, Aug 28, 2012 at 9:17 PM, William Cohen <wc...@re...> wrote: >> On 08/28/2012 12:03 PM, stephane eranian wrote: >>> On Tue, Aug 28, 2012 at 4:45 PM, William Cohen <wc...@re...> wrote: >> >>>> >>>> Would it be possible to do the ldconf separately from the "make install"? The rpm packages are built as normal users with a staged install and the ldconf is done as a post install operation. The ldconf in the makefile needed to be disabled for the libpfm rpm to be built. >>>> >>> I just updated the libpfm.spec today to convert ldconfig to a nop >>> (LDCONFIG=true), so >>> it should not perturb the generation of the RPM. At least, it did not >>> for me when I tried >>> on Fedora 16. >>> >> >> Hi Stephane, >> >> >> Thanks for the libpfm.spec. The fedora rawhide libpfm-4.3.0 doesn't have any patches in it now: >> >> http://koji.fedoraproject.org/koji/taskinfo?taskID=4432221 >> >> I ran libpfm through coverity to see whether there were any serious problems with it. Attached is the output of coverity. All errors are in the generated swig code and the examples. Probably can't do much about the swig related error because there are all in generated code. The examples could be cleaned up a little, but those are not packaged in the libpfm rpms, so those are not a great problem. >> >> In perf_setup_list_events() in perf_util.c why not make the for loop replace the ',' with '\0' the same structure as the earlier while loop counting the args? That would make the code a bit easier to read and eliminate one of the coverity errors. >> . >> -Will Hi Stephane, The results are improved. Most all the ones remaining are in the python swig wrappers. The one that isn't in the swig wrappers is: Error: NULL_RETURNS (CWE-476): /builddir/build/BUILD/libpfm-4.3.0/examples/showevtinfo.c:659: example_assign: Assigning: "p" = return value from "strchr(arg, 44)". /builddir/build/BUILD/libpfm-4.3.0/examples/showevtinfo.c:660: example_checked: "p" has its value checked in "p". /builddir/build/BUILD/libpfm-4.3.0/lib/pfmlib_common.c:476: example_assign: Assigning: "p" = return value from "strchr(pfm_cfg.forced_pmu, 44)". /builddir/build/BUILD/libpfm-4.3.0/lib/pfmlib_common.c:477: example_checked: "p" has its value checked in "p". /builddir/build/BUILD/libpfm-4.3.0/lib/pfmlib_common.c:970: example_assign: Assigning: "p" = return value from "strchr(s, 44)". /builddir/build/BUILD/libpfm-4.3.0/lib/pfmlib_common.c:971: example_checked: "p" has its value checked in "p". /builddir/build/BUILD/libpfm-4.3.0/perf_examples/perf_util.c:127: example_checked: "strchr(q, 44)" has its value checked in "p = strchr(q, 44)". /builddir/build/BUILD/libpfm-4.3.0/perf_examples/perf_util.c:141: returned_null: Function "strchr" returns null (checked 4 out of 5 times). /builddir/build/BUILD/libpfm-4.3.0/perf_examples/perf_util.c:141: var_assigned: Assigning: "p" = null return value from "strchr". /builddir/build/BUILD/libpfm-4.3.0/perf_examples/perf_util.c:142: dereference: Dereferencing a null pointer "p". Earlier in the code have: q = events; while((p = strchr(q, ','))) { num++; q = p + 1; } num++; num++; /* term Why not code the following for loop: for(i=0, q = events; i < num-2; i++, q = p + 1) { p = strchr(q, ','); *p = '\0'; argv[i] = q; } argv[i++] = q; argv[i] = NULL; to mirror the earlier loop so the strchr value is checked? So make the for loop into something like: q = events; i = 0; while((p = strchr(q, ','))) { *p = '\0'; i++; q = p + 1; } argv[i++] = q; argv[i] =NULL; /* terminator */ -Will |
From: William C. <wc...@re...> - 2012-09-04 19:31:59
Attachments:
0001-Simplify-perf_util.c-loop.patch
|
On 09/04/2012 10:25 AM, William Cohen wrote: > On 09/03/2012 10:23 AM, stephane eranian wrote: >> Will, >> >> I have fixed most of the warnings reported by your tool. >> Please pull and try again. >> >> >> On Tue, Aug 28, 2012 at 9:17 PM, William Cohen <wc...@re...> wrote: >>> On 08/28/2012 12:03 PM, stephane eranian wrote: >>>> On Tue, Aug 28, 2012 at 4:45 PM, William Cohen <wc...@re...> wrote: >>> >>>>> >>>>> Would it be possible to do the ldconf separately from the "make install"? The rpm packages are built as normal users with a staged install and the ldconf is done as a post install operation. The ldconf in the makefile needed to be disabled for the libpfm rpm to be built. >>>>> >>>> I just updated the libpfm.spec today to convert ldconfig to a nop >>>> (LDCONFIG=true), so >>>> it should not perturb the generation of the RPM. At least, it did not >>>> for me when I tried >>>> on Fedora 16. >>>> >>> >>> Hi Stephane, >>> >>> >>> Thanks for the libpfm.spec. The fedora rawhide libpfm-4.3.0 doesn't have any patches in it now: >>> >>> http://koji.fedoraproject.org/koji/taskinfo?taskID=4432221 >>> >>> I ran libpfm through coverity to see whether there were any serious problems with it. Attached is the output of coverity. All errors are in the generated swig code and the examples. Probably can't do much about the swig related error because there are all in generated code. The examples could be cleaned up a little, but those are not packaged in the libpfm rpms, so those are not a great problem. >>> >>> In perf_setup_list_events() in perf_util.c why not make the for loop replace the ',' with '\0' the same structure as the earlier while loop counting the args? That would make the code a bit easier to read and eliminate one of the coverity errors. >>> . >>> -Will > > Hi Stephane, > > The results are improved. Most all the ones remaining are in the python swig wrappers. The one that isn't in the swig wrappers is: > > Error: NULL_RETURNS (CWE-476): > /builddir/build/BUILD/libpfm-4.3.0/examples/showevtinfo.c:659: example_assign: Assigning: "p" = return value from "strchr(arg, 44)". > /builddir/build/BUILD/libpfm-4.3.0/examples/showevtinfo.c:660: example_checked: "p" has its value checked in "p". > /builddir/build/BUILD/libpfm-4.3.0/lib/pfmlib_common.c:476: example_assign: Assigning: "p" = return value from "strchr(pfm_cfg.forced_pmu, 44)". > /builddir/build/BUILD/libpfm-4.3.0/lib/pfmlib_common.c:477: example_checked: "p" has its value checked in "p". > /builddir/build/BUILD/libpfm-4.3.0/lib/pfmlib_common.c:970: example_assign: Assigning: "p" = return value from "strchr(s, 44)". > /builddir/build/BUILD/libpfm-4.3.0/lib/pfmlib_common.c:971: example_checked: "p" has its value checked in "p". > /builddir/build/BUILD/libpfm-4.3.0/perf_examples/perf_util.c:127: example_checked: "strchr(q, 44)" has its value checked in "p = strchr(q, 44)". > /builddir/build/BUILD/libpfm-4.3.0/perf_examples/perf_util.c:141: returned_null: Function "strchr" returns null (checked 4 out of 5 times). > /builddir/build/BUILD/libpfm-4.3.0/perf_examples/perf_util.c:141: var_assigned: Assigning: "p" = null return value from "strchr". > /builddir/build/BUILD/libpfm-4.3.0/perf_examples/perf_util.c:142: dereference: Dereferencing a null pointer "p". > > > Earlier in the code have: > > q = events; > while((p = strchr(q, ','))) { > num++; > q = p + 1; > } > num++; > num++; /* term > > Why not code the following for loop: > > > for(i=0, q = events; i < num-2; i++, q = p + 1) { > p = strchr(q, ','); > *p = '\0'; > argv[i] = q; > } > argv[i++] = q; > argv[i] = NULL; > > to mirror the earlier loop so the strchr value is checked? So make the for loop into something like: > > q = events; > i = 0; > while((p = strchr(q, ','))) { > *p = '\0'; > i++; > q = p + 1; > } > argv[i++] = q; > argv[i] =NULL; /* terminator */ > > -Will > The previous suggested code is not correct (missing the actual assignment to argv[i]). Attached is a patch that attempts to simplify the code and remove the coverity error. Before the patch have the following errors: grep ^E libpfm-4.3.0-2.fc17cov2/run0/libpfm-4.3.0-2.fc17cov2.err |sort |uniq -c 1 Error: DEADCODE (CWE-561): 1 Error: NULL_RETURNS (CWE-476): 4 Error: RESOURCE_LEAK (CWE-404): 4 Error: UNUSED_VALUE (CWE-563): After the patch have the following errors: $ grep ^E libpfm-4.3.0-2.fc17cov2/run1/libpfm-4.3.0-2.fc17cov2.err |sort |uniq -c 1 Error: DEADCODE (CWE-561): 4 Error: RESOURCE_LEAK (CWE-404): 4 Error: UNUSED_VALUE (CWE-563): -Will |
From: stephane e. <er...@go...> - 2012-09-05 07:16:57
|
On Tue, Sep 4, 2012 at 4:25 PM, William Cohen <wc...@re...> wrote: > On 09/03/2012 10:23 AM, stephane eranian wrote: >> Will, >> >> I have fixed most of the warnings reported by your tool. >> Please pull and try again. >> >> >> On Tue, Aug 28, 2012 at 9:17 PM, William Cohen <wc...@re...> wrote: >>> On 08/28/2012 12:03 PM, stephane eranian wrote: >>>> On Tue, Aug 28, 2012 at 4:45 PM, William Cohen <wc...@re...> wrote: >>> >>>>> >>>>> Would it be possible to do the ldconf separately from the "make install"? The rpm packages are built as normal users with a staged install and the ldconf is done as a post install operation. The ldconf in the makefile needed to be disabled for the libpfm rpm to be built. >>>>> >>>> I just updated the libpfm.spec today to convert ldconfig to a nop >>>> (LDCONFIG=true), so >>>> it should not perturb the generation of the RPM. At least, it did not >>>> for me when I tried >>>> on Fedora 16. >>>> >>> >>> Hi Stephane, >>> >>> >>> Thanks for the libpfm.spec. The fedora rawhide libpfm-4.3.0 doesn't have any patches in it now: >>> >>> http://koji.fedoraproject.org/koji/taskinfo?taskID=4432221 >>> >>> I ran libpfm through coverity to see whether there were any serious problems with it. Attached is the output of coverity. All errors are in the generated swig code and the examples. Probably can't do much about the swig related error because there are all in generated code. The examples could be cleaned up a little, but those are not packaged in the libpfm rpms, so those are not a great problem. >>> >>> In perf_setup_list_events() in perf_util.c why not make the for loop replace the ',' with '\0' the same structure as the earlier while loop counting the args? That would make the code a bit easier to read and eliminate one of the coverity errors. >>> . >>> -Will > > Hi Stephane, > > The results are improved. Most all the ones remaining are in the python swig wrappers. The one that isn't in the swig wrappers is: > > Error: NULL_RETURNS (CWE-476): > /builddir/build/BUILD/libpfm-4.3.0/examples/showevtinfo.c:659: example_assign: Assigning: "p" = return value from "strchr(arg, 44)". > /builddir/build/BUILD/libpfm-4.3.0/examples/showevtinfo.c:660: example_checked: "p" has its value checked in "p". > /builddir/build/BUILD/libpfm-4.3.0/lib/pfmlib_common.c:476: example_assign: Assigning: "p" = return value from "strchr(pfm_cfg.forced_pmu, 44)". > /builddir/build/BUILD/libpfm-4.3.0/lib/pfmlib_common.c:477: example_checked: "p" has its value checked in "p". I don't understand what's wrong with p = strchr(); if (p) .... The return value is checked. Unless you're telling me that Coverity does not like that because it expects if (p != NULL) ... > /builddir/build/BUILD/libpfm-4.3.0/lib/pfmlib_common.c:970: example_assign: Assigning: "p" = return value from "strchr(s, 44)". > /builddir/build/BUILD/libpfm-4.3.0/lib/pfmlib_common.c:971: example_checked: "p" has its value checked in "p". > /builddir/build/BUILD/libpfm-4.3.0/perf_examples/perf_util.c:127: example_checked: "strchr(q, 44)" has its value checked in "p = strchr(q, 44)". > /builddir/build/BUILD/libpfm-4.3.0/perf_examples/perf_util.c:141: returned_null: Function "strchr" returns null (checked 4 out of 5 times). > /builddir/build/BUILD/libpfm-4.3.0/perf_examples/perf_util.c:141: var_assigned: Assigning: "p" = null return value from "strchr". > /builddir/build/BUILD/libpfm-4.3.0/perf_examples/perf_util.c:142: dereference: Dereferencing a null pointer "p". > > > Earlier in the code have: > > q = events; > while((p = strchr(q, ','))) { > num++; > q = p + 1; > } > num++; > num++; /* term > > Why not code the following for loop: > > > for(i=0, q = events; i < num-2; i++, q = p + 1) { > p = strchr(q, ','); > *p = '\0'; > argv[i] = q; > } > argv[i++] = q; > argv[i] = NULL; > This code is correct. It's just that Coverity cannot follow the logic of it (num-2). But I am willing to simplify it. > to mirror the earlier loop so the strchr value is checked? So make the for loop into something like: > > q = events; > i = 0; > while((p = strchr(q, ','))) { > *p = '\0'; > i++; > q = p + 1; > } > argv[i++] = q; > argv[i] =NULL; /* terminator */ > > -Will |
From: stephane e. <er...@go...> - 2012-09-05 07:40:01
|
Will, I have applied your patch. What is left to fix from the Coverity output? On Wed, Sep 5, 2012 at 9:16 AM, stephane eranian <er...@go...> wrote: > On Tue, Sep 4, 2012 at 4:25 PM, William Cohen <wc...@re...> wrote: >> On 09/03/2012 10:23 AM, stephane eranian wrote: >>> Will, >>> >>> I have fixed most of the warnings reported by your tool. >>> Please pull and try again. >>> >>> >>> On Tue, Aug 28, 2012 at 9:17 PM, William Cohen <wc...@re...> wrote: >>>> On 08/28/2012 12:03 PM, stephane eranian wrote: >>>>> On Tue, Aug 28, 2012 at 4:45 PM, William Cohen <wc...@re...> wrote: >>>> >>>>>> >>>>>> Would it be possible to do the ldconf separately from the "make install"? The rpm packages are built as normal users with a staged install and the ldconf is done as a post install operation. The ldconf in the makefile needed to be disabled for the libpfm rpm to be built. >>>>>> >>>>> I just updated the libpfm.spec today to convert ldconfig to a nop >>>>> (LDCONFIG=true), so >>>>> it should not perturb the generation of the RPM. At least, it did not >>>>> for me when I tried >>>>> on Fedora 16. >>>>> >>>> >>>> Hi Stephane, >>>> >>>> >>>> Thanks for the libpfm.spec. The fedora rawhide libpfm-4.3.0 doesn't have any patches in it now: >>>> >>>> http://koji.fedoraproject.org/koji/taskinfo?taskID=4432221 >>>> >>>> I ran libpfm through coverity to see whether there were any serious problems with it. Attached is the output of coverity. All errors are in the generated swig code and the examples. Probably can't do much about the swig related error because there are all in generated code. The examples could be cleaned up a little, but those are not packaged in the libpfm rpms, so those are not a great problem. >>>> >>>> In perf_setup_list_events() in perf_util.c why not make the for loop replace the ',' with '\0' the same structure as the earlier while loop counting the args? That would make the code a bit easier to read and eliminate one of the coverity errors. >>>> . >>>> -Will >> >> Hi Stephane, >> >> The results are improved. Most all the ones remaining are in the python swig wrappers. The one that isn't in the swig wrappers is: >> >> Error: NULL_RETURNS (CWE-476): >> /builddir/build/BUILD/libpfm-4.3.0/examples/showevtinfo.c:659: example_assign: Assigning: "p" = return value from "strchr(arg, 44)". >> /builddir/build/BUILD/libpfm-4.3.0/examples/showevtinfo.c:660: example_checked: "p" has its value checked in "p". >> /builddir/build/BUILD/libpfm-4.3.0/lib/pfmlib_common.c:476: example_assign: Assigning: "p" = return value from "strchr(pfm_cfg.forced_pmu, 44)". >> /builddir/build/BUILD/libpfm-4.3.0/lib/pfmlib_common.c:477: example_checked: "p" has its value checked in "p". > > I don't understand what's wrong with p = strchr(); if (p) .... The > return value is checked. Unless you're telling me > that Coverity does not like that because it expects if (p != NULL) ... > > >> /builddir/build/BUILD/libpfm-4.3.0/lib/pfmlib_common.c:970: example_assign: Assigning: "p" = return value from "strchr(s, 44)". >> /builddir/build/BUILD/libpfm-4.3.0/lib/pfmlib_common.c:971: example_checked: "p" has its value checked in "p". >> /builddir/build/BUILD/libpfm-4.3.0/perf_examples/perf_util.c:127: example_checked: "strchr(q, 44)" has its value checked in "p = strchr(q, 44)". >> /builddir/build/BUILD/libpfm-4.3.0/perf_examples/perf_util.c:141: returned_null: Function "strchr" returns null (checked 4 out of 5 times). >> /builddir/build/BUILD/libpfm-4.3.0/perf_examples/perf_util.c:141: var_assigned: Assigning: "p" = null return value from "strchr". >> /builddir/build/BUILD/libpfm-4.3.0/perf_examples/perf_util.c:142: dereference: Dereferencing a null pointer "p". >> >> >> Earlier in the code have: >> >> q = events; >> while((p = strchr(q, ','))) { >> num++; >> q = p + 1; >> } >> num++; >> num++; /* term >> >> Why not code the following for loop: >> >> >> for(i=0, q = events; i < num-2; i++, q = p + 1) { >> p = strchr(q, ','); >> *p = '\0'; >> argv[i] = q; >> } >> argv[i++] = q; >> argv[i] = NULL; >> > This code is correct. It's just that Coverity cannot follow the logic > of it (num-2). > But I am willing to simplify it. > >> to mirror the earlier loop so the strchr value is checked? So make the for loop into something like: >> >> q = events; >> i = 0; >> while((p = strchr(q, ','))) { >> *p = '\0'; >> i++; >> q = p + 1; >> } >> argv[i++] = q; >> argv[i] =NULL; /* terminator */ >> >> -Will |
From: William C. <wc...@re...> - 2012-09-05 13:34:43
|
On 09/05/2012 03:39 AM, stephane eranian wrote: > Will, > > > I have applied your patch. > What is left to fix from the Coverity output? Hi Stephane, Thanks. Coverity doesn't seems to be smart enough to determine there are enough ',' in the string to eliminate the possibility of getting NULL value returned by strchr(). That removes the last of the coverity errors in human generated code. The rest of the errors are in the the machine generated perfmon_int_wrap.c. -Will |