[PATCH 0/2] version3, Add time stamp collection to operf and opreport,

Will:

Sorry for the long time in getting these patches updated.  Your comments
of version 2 are below.  I believe that I have addressed all of the
comments.  The most significant being the use of gnuplot.  You suggested
drawing the graphs using SVG instead.  I wrote a new C program that
takes the data from the perl script that extracts the data from the
OProfile XML output file and draws the graphs using SVG.  This is really
the most significant change to the patches.  The other comments required
relatively small changes.  

I have tested opening the SVG graph file on a few different platforms
and browsers.  I tested it using Mozilla Firefox on a RedHat linux 7.2
distribution, on Mozilla Firefox and Chrome on a current Mac Air machine
and on Mozilla, Chrome and IE on an old Window Vista box, Mozilla
Firefox on a Windows XP machine.  The all of the browsers on these
machines were able to properly open and display graphs correctly. 

Please let me know if you see any issues that need additional work.
Thank you.  

No rush, I know these are rather large patches to be reviewed.  

                      Carl Love

On Tue, 2015-11-17 at 22:05 -0500, William Cohen wrote:
> 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



------------------------------------------------------------------------------
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785351&iu=/4140