Re: [PATCH 1/2 RFC] version2, Add time stamp collection to operf and opreport

On 10/22/2015 07:22 PM, Carl E. Love wrote:
> Updated version 2,  Add time stamp collection to operf, report time stamps in XML output
> 
> This patch adds a command line option to operf to collect timestamp data
> for each sample.  The time stamps are printed to the various oprofile data
> files.
> 
> The opreport command also has a command line option added.  This option
> will print the time stamp data in the XML output file for each sample.  If
> the -d option is used the time stamps will also be printed for each
> instruct in the detailed XML output file.
> 
> The initial version of the patch had some issues supporting the printing
> of the time stamps on multiple events.  Thes have been fixed in this
> version.
> 
> Fixed an issue where there were duplicate entries for the-T option.  The
> second one was removed.  The variable that it set was not used.
> 
> Add support to opreport to call graphing scripts
> 
> Signed-off-by: Carl Love <[email protected]>

Hi Carl,

It is taking some time to look over the 3000 lines worth of patches.

I used ran the linux kernel's checkpatch.pl and the
oprofile-tests/check_style.py scripts over the patches.  There are a
number of places with trailing whitespace that should be elminated
from the patches.

Some printf in the patches don't work on 32-bit processors and need to be made portable:

make[3]: Entering directory '/builddir/build/BUILD/oprofile-1.2.0git/libdb'
gcc -DHAVE_CONFIG_H -I. -I..  -I ../libop -I ../libutil   -W -Wall -fno-common -Wdeclaration-after-statement -Werror -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m32 -march=i686 -mtune=atom -fasynchronous-unwind-tables -c -o db_manage.o db_manage.c
db_manage.c: In function 'read_timestamp':
db_manage.c:311:20: error: format '%ld' expects argument of type 'long int', but argument 3 has type 'uint64_t {aka long long unsigned int}' [-Werror=format=]
    fprintf(stderr, "ERROR, failed to read enough bytes for "
                    ^
db_manage.c: In function 'odb_write_timestamps':
db_manage.c:419:20: error: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'size_t {aka unsigned int}' [-Werror=format=]
    fprintf(stderr, "WARNING: missing time stamps "
                    ^
cc1: all warnings being treated as errors



The patches apply cleanly to the current git repository.  However,
when trying to build a Fedora RPM I got an error about an unchecked
return value:

Making all in pp
make[2]: Entering directory '/home/wcohen/rpmbuild/BUILD/oprofile-1.2.0git/pp'
g++ -DHAVE_CONFIG_H -I. -I..  -I ../libop -I ../libutil -I ../libdb -I ../libopt++ -I ../libutil++ -I ../libregex -I ../libpp   -W -Wall -fno-common -ftemplate-depth-50 -Werror -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches  -m64 -mtune=generic -c -o opreport.o opreport.cpp
opreport.cpp: In function 'int {anonymous}::opreport(const options::spec&)':
opreport.cpp:601:46: error: ignoring return value of 'int system(const char*)', declared with attribute warn_unused_result [-Werror=unused-result]
   system(options::time_stamp_options.c_str());
                                              ^
cc1plus: all warnings being treated as errors


Should the code be throwing a error if the system() command returns a
non-zero value?  Something like the patch is the scratchbuild at:

http://koji.fedoraproject.org/koji/taskinfo?taskID=11888018


Is the following statement in opreport.pp leftover from debugging:

		cout << "CARLL COMMAND = " << options::time_stamp_options
			<< endl;


A run of the oprofile rpm with the patches through coverity looks good
(no additional coverity errors)

There are going to be some machine such as embedded platforms where
gnuplot may not be available or not installed. Has the code been
verified to handle this case in a reasonable manner?

Rather than writing out postscript would it make more sense to generate SVG?
Most webbrowsers can read SVG file directly.

In the doc/plot_time_stamps.1.in shouldn't the synopsis be
plot_time_stamps.sh rather than process_time_stamps.sh.  There isn't a
process_time_stamps.sh command (there is a process_time_stamps.prl
file).

If the opreport -T option must be used with used with --xml, why not
force setting the --xml when -T is used?

When running the example "opreport -X -T -P" the process cored after
putting up the plot probably should eliminate the diagnostic messages
in plot_time_stamps.sh:

--------
$ opreport -X -T -P
Using /home/wcohen/rpmbuild/SPECS/oprofile_data/samples/ for samples directory.
warning: /dm_crypt could not be found.
warning: /e1000e could not be found.
warning: /iptable_mangle could not be found.
warning: /kvm could not be found.
warning: /nf_conntrack could not be found.
warning: /nf_conntrack_ipv4 could not be found.
warning: /nf_nat could not be found.
warning: /nouveau could not be found.
warning: /xt_conntrack could not be found.
 
 Processing XML time stamp data from file opreport.xml... 
 
 Done processing XML time stamp data.
 
 Calling gnuplot to create postscript file.  Output file is:  time_stamp_plot.ps .
 

set mytics 0
           ^
"./workload_tm_cmd.plt", line 9: warning: Expecting number of intervals
 Calling xdg-open to open the postscript file  time_stamp_plot.ps .
 
Segmentation fault (core dumped)
--------

The followin are defined in both libop/tm_array.h and
libop/op_config.h. probably should just be in libop/tm_array.h:

+#define EMPTY_TM_ARRAY  ~0x0ULL
+#define VALID_TM_ARRAY_END  0xA5A5A5A55A5A5A5AULL

--------

Should libdb/db_manage.c only be including tm_array.h?

--------

It looks like DEFAULT_OPREPORT_XML_NAME opreport.xml is always being
used for in handle_time_stamp_options. Should it be using some sort of
temporary file or allow selecting the file name (-o) being written out
if plot being generated directly from opreport?


------------------

plot_time_stamps.sh always uses process_time_stamps.out as a temporary
file when running process_time_stamps.prl.  Should plot_time_stamps.sh
be using some temporary file instead?  Same for workload_tm_data.gpi
and workload_tm_cmd.plt?

plot_time_stamps.sh always calls xdg-open if xdg-open is on the
system.  If someone is running plot_time_stamps.sh with --ps_file,
they probably don't want to have a window pop up on the screen.


-Will

------------------------------------------------------------------------------