Menu
▾
▴
Re: [PATCH] Avoid changing the number formatting for cout and cerr streams
|
From: Maynard J. <may...@us...> - 2013-06-04 14:14:32
|
On 06/03/2013 12:33 PM, William Cohen wrote:
> Coverity static tool found a number of places where number formatting
> was changed and might lead to some oddly formatted output for later
> stream output. This patch ensures that the number formatting only
> applies to that particular message.
Hi, Will,
This is a nice, clever way of solving the stream format change issue Coverity complains about. Thanks for coming up with this solution. However, when passing the ostringstream object to ostream << operator, the object needs to be string'ized (i.e., message.str()); otherwise, the only thing that's printed is a hex value, which I take to be the address of the object. I can easily massage your two patches to add that call to str(). But perhaps you would rather re-submit them. Just let me know.
-Maynard
>
> Signed-off-by: William Cohen <wc...@re...>
> ---
> libabi/opimport.cpp | 8 +++++--
> libperf_events/operf_counter.cpp | 19 ++++++++++++----
> libperf_events/operf_kernel.cpp | 9 ++++++--
> libperf_events/operf_process_info.cpp | 20 +++++++++++-----
> libperf_events/operf_sfile.cpp | 8 +++++--
> libperf_events/operf_utils.cpp | 43 +++++++++++++++++++++++++----------
> libutil++/bfd_support.cpp | 5 +++-
> 7 files changed, 82 insertions(+), 30 deletions(-)
>
> diff --git a/libabi/opimport.cpp b/libabi/opimport.cpp
> index 64c3bd0..d931770 100644
> --- a/libabi/opimport.cpp
> +++ b/libabi/opimport.cpp
> @@ -15,6 +15,7 @@
>
> #include <fstream>
> #include <iostream>
> +#include <sstream>
> #include <vector>
> #include <cassert>
> #include <cstring>
> @@ -87,10 +88,13 @@ void extractor::extract(T & targ, void const * src_,
> assert(src >= begin);
> assert(src + nbytes <= end);
>
> - if (verbose)
> - cerr << hex << "get " << sz << " = " << nbytes
> + if (verbose) {
> + ostringstream message;
> + message << hex << "get " << sz << " = " << nbytes
> << " bytes @ " << off << " = " << (src - begin)
> << " : ";
> + cerr << message;
> + }
>
> if (little_endian)
> while(nbytes--)
> diff --git a/libperf_events/operf_counter.cpp b/libperf_events/operf_counter.cpp
> index 4f20e4a..9d47e26 100644
> --- a/libperf_events/operf_counter.cpp
> +++ b/libperf_events/operf_counter.cpp
> @@ -22,6 +22,7 @@
> #include <errno.h>
> #include <string.h>
> #include <iostream>
> +#include <sstream>
> #include <stdlib.h>
> #include "op_events.h"
> #include "operf_counter.h"
> @@ -433,8 +434,10 @@ void operf_record::register_perf_event_id(unsigned event, u64 id, perf_event_att
> // is invoked once for each event for each cpu; but it's not worth the bother of trying
> // to avoid it.
> opHeader.h_attrs[event].attr = attr;
> - cverb << vrecord << "Perf header: id = " << hex << (unsigned long long)id << " for event num "
> + ostringstream message;
> + message << "Perf header: id = " << hex << (unsigned long long)id << " for event num "
> << event << ", code " << attr.config << endl;
> + cverb << vrecord << message.str();
> opHeader.h_attrs[event].ids.push_back(id);
> }
>
> @@ -836,7 +839,9 @@ int operf_read::_read_header_info_with_ifstream(void)
> ret = OP_PERF_HANDLED_ERROR;
> goto out;
> }
> - cverb << vconvert << "Perf header: id = " << hex << (unsigned long long)perf_id << endl;
> + ostringstream message;
> + message << "Perf header: id = " << hex << (unsigned long long)perf_id << endl;
> + cverb << vconvert << message;
> opHeader.h_attrs[i].ids.push_back(perf_id);
> }
> istrm.seekg(next_f_attr, ios_base::beg);
> @@ -926,7 +931,9 @@ int operf_read::_read_perf_header_from_pipe(void)
> errmsg = "Error reading perf ID on sample data pipe: " + string(strerror(errno));
> goto fail;
> }
> - cverb << vconvert << "Perf header: id = " << hex << (unsigned long long)perf_id << endl;
> + ostringstream message;
> + message << "Perf header: id = " << hex << (unsigned long long)perf_id << endl;
> + cverb << vconvert << message;
> opHeader.h_attrs[i].ids.push_back(perf_id);
> }
>
> @@ -994,8 +1001,10 @@ unsigned int operf_read::convertPerfData(void)
> for (int i = 0; i < OPERF_MAX_STATS; i++)
> operf_stats[i] = 0;
>
> - cverb << vdebug << "Converting operf data to oprofile sample data format" << endl;
> - cverb << vdebug << "sample type is " << hex << opHeader.h_attrs[0].attr.sample_type << endl;
> + ostringstream message;
> + message << "Converting operf data to oprofile sample data format" << endl;
> + message << "sample type is " << hex << opHeader.h_attrs[0].attr.sample_type << endl;
> + cverb << vdebug << message;
> first_time_processing = true;
> int num_recs = 0;
> struct perf_event_header last_header;
> diff --git a/libperf_events/operf_kernel.cpp b/libperf_events/operf_kernel.cpp
> index dbe22de..e7b522f 100644
> --- a/libperf_events/operf_kernel.cpp
> +++ b/libperf_events/operf_kernel.cpp
> @@ -13,6 +13,7 @@
>
> #include <stdio.h>
> #include <iostream>
> +#include <sstream>
> #include <unistd.h>
> #include <stdlib.h>
> #include "operf_kernel.h"
> @@ -47,12 +48,16 @@ void operf_create_vmlinux(char const * name, char const * arg)
>
> sscanf(arg, "%llx,%llx", &vmlinux_image.start, &vmlinux_image.end);
>
> - cverb << vmisc << "kernel_start = " << hex << vmlinux_image.start
> + ostringstream message;
> + message << "kernel_start = " << hex << vmlinux_image.start
> << "; kernel_end = " << vmlinux_image.end << endl;
> + cverb << vmisc << message;
>
> if (!vmlinux_image.start && !vmlinux_image.end) {
> - cerr << "error: mis-parsed kernel range: " << hex << vmlinux_image.start
> + ostringstream message;
> + message << "error: mis-parsed kernel range: " << hex << vmlinux_image.start
> << "; kernel_end = " << vmlinux_image.end << endl;
> + cerr << message;
> exit(EXIT_FAILURE);
> }
> }
> diff --git a/libperf_events/operf_process_info.cpp b/libperf_events/operf_process_info.cpp
> index 5f9351a..3bbfdcc 100644
> --- a/libperf_events/operf_process_info.cpp
> +++ b/libperf_events/operf_process_info.cpp
> @@ -18,6 +18,7 @@
> #include <stdio.h>
> #include <unistd.h>
> #include <iostream>
> +#include <sstream>
> #include <map>
> #include <string.h>
> #include <errno.h>
> @@ -98,9 +99,12 @@ void operf_process_info::set_appname(const char * appname, bool app_arg_is_fulln
> /* Most likely that the process has ended already, so we'll need to determine
> * the appname through different means.
> */
> - if (cverb << vmisc)
> - cerr << "PID: " << hex << pid << " Unable to obtain appname from " << exe_symlink << endl
> + if (cverb << vmisc) {
> + ostringstream message;
> + message << "PID: " << hex << pid << " Unable to obtain appname from " << exe_symlink << endl
> << "\t" << strerror(errno) << endl;
> + cerr << message;
> + }
> if (appname && strcmp(appname, "taskset")) {
> _appname = appname;
> if (app_arg_is_fullname) {
> @@ -113,8 +117,10 @@ void operf_process_info::set_appname(const char * appname, bool app_arg_is_fulln
> }
> app_basename = _appname;
> }
> - cverb << vmisc << "PID: " << hex << pid << " appname is set to "
> + ostringstream message;
> + message << "PID: " << hex << pid << " appname is set to "
> << _appname << endl;
> + cverb << vmisc << message;
> if (look_for_appname_match)
> find_best_match_appname_all_mappings();
> }
> @@ -278,9 +284,11 @@ void operf_process_info::process_hypervisor_mapping(u64 ip)
> hypervisor_mmap->pgoff = 0;
> hypervisor_mmap->is_hypervisor = true;
> if (cverb << vmisc) {
> - cout << "Synthesize mmapping for " << hypervisor_mmap->filename << endl;
> - cout << "\tstart_addr: " << hex << hypervisor_mmap->start_addr;
> - cout << "; end addr: " << hypervisor_mmap->end_addr << endl;
> + ostringstream message;
> + message << "Synthesize mmapping for " << hypervisor_mmap->filename << endl;
> + message << "\tstart_addr: " << hex << hypervisor_mmap->start_addr;
> + message << "; end addr: " << hypervisor_mmap->end_addr << endl;
> + cout << message;
> }
> process_mapping(hypervisor_mmap, false);
> }
> diff --git a/libperf_events/operf_sfile.cpp b/libperf_events/operf_sfile.cpp
> index 595715a..ea1f6e1 100644
> --- a/libperf_events/operf_sfile.cpp
> +++ b/libperf_events/operf_sfile.cpp
> @@ -15,6 +15,7 @@
> #include <string.h>
> #include <assert.h>
> #include <iostream>
> +#include <sstream>
>
> #include "operf_sfile.h"
> #include "operf_kernel.h"
> @@ -187,8 +188,11 @@ struct operf_sfile * operf_sfile_find(struct operf_transient const * trans)
> if (trans->in_kernel) {
> ki = operf_find_kernel_image(trans->pc);
> if (!ki) {
> - if (cverb << vsfile)
> - cout << "Lost kernel sample " << std::hex << trans->pc << std::endl;;
> + if (cverb << vsfile) {
> + ostringstream message;
> + message << "Lost kernel sample " << std::hex << trans->pc << std::endl;;
> + cout << message;
> + }
> operf_stats[OPERF_LOST_KERNEL]++;
> return NULL;
> }
> diff --git a/libperf_events/operf_utils.cpp b/libperf_events/operf_utils.cpp
> index f7e0ea6..fd14083 100644
> --- a/libperf_events/operf_utils.cpp
> +++ b/libperf_events/operf_utils.cpp
> @@ -22,6 +22,7 @@
> #include <fcntl.h>
> #include <cverb.h>
> #include <iostream>
> +#include <sstream>
> #include "operf_counter.h"
> #include "operf_utils.h"
> #ifdef HAVE_LIBPFM
> @@ -588,9 +589,11 @@ static struct operf_transient * __get_operf_trans(struct sample_data * data, boo
> } else {
> if ((cverb << vconvert) && !first_time_processing) {
> string domain = trans.in_kernel ? "kernel" : "userspace";
> - cout << "Discarding " << domain << " sample for process " << data->pid
> + ostringstream message;
> + message << "Discarding " << domain << " sample for process " << data->pid
> << " where no appropriate mapping was found. (pc=0x"
> << hex << data->ip <<")" << endl;
> + cout << message;
> operf_stats[OPERF_LOST_NO_MAPPING]++;
> }
> retval = NULL;
> @@ -775,8 +778,10 @@ static int __handle_sample_event(event_t * event, u64 sample_type)
> domain = "unknown";
> break;
> }
> - cout << "Discarding sample from " << domain << " domain: "
> + ostringstream message;
> + message << "Discarding sample from " << domain << " domain: "
> << hex << data.ip << endl;
> + cout << message;
> }
> goto out;
> }
> @@ -792,10 +797,13 @@ static int __handle_sample_event(event_t * event, u64 sample_type)
> goto out;
> }
>
> - if (cverb << vconvert)
> - cout << "(IP, " << event->header.misc << "): " << dec << data.pid << "/"
> + if (cverb << vconvert) {
> + ostringstream message;
> + message << "(IP, " << event->header.misc << "): " << dec << data.pid << "/"
> << data.tid << ": " << hex << (unsigned long long)data.ip
> << endl << "\tdata ID: " << data.id << endl;
> + cout << message;
> + }
>
> // Verify the sample.
> if (data.id != trans.sample_id) {
> @@ -939,12 +947,16 @@ int OP_perf_utils::op_write_event(event_t * event, u64 sample_type)
> default:
> if (event->header.type > PERF_RECORD_MAX) {
> // Bad header
> - cerr << "Invalid event type " << hex << event->header.type << endl;
> - cerr << "Sample data is probably corrupted." << endl;
> + ostringstream message;
> + message << "Invalid event type " << hex << event->header.type << endl;
> + message << "Sample data is probably corrupted." << endl;
> + cerr << message;
> return -1;
> } else {
> - cverb << vconvert << "Event type "<< hex << event->header.type
> + ostringstream message;
> + message << "Event type "<< hex << event->header.type
> << " is ignored." << endl;
> + cverb << vconvert << message;
> return 0;
> }
> }
> @@ -1036,14 +1048,18 @@ static int __mmap_trace_file(struct mmap_info & info)
> info.buf = (char *) mmap(NULL, mmap_size, mmap_prot,
> mmap_flags, info.traceFD, info.offset);
> if (info.buf == MAP_FAILED) {
> - cerr << "Error: mmap failed with errno:\n\t" << strerror(errno) << endl;
> - cerr << "\tmmap_size: 0x" << hex << mmap_size << "; offset: 0x" << info.offset << endl;
> + ostringstream message;
> + message << "Error: mmap failed with errno:\n\t" << strerror(errno) << endl;
> + message << "\tmmap_size: 0x" << hex << mmap_size << "; offset: 0x" << info.offset << endl;
> + cerr << message;
> return -1;
> }
> else {
> - cverb << vconvert << hex << "mmap with the following parameters" << endl
> + ostringstream message;
> + message << hex << "mmap with the following parameters" << endl
> << "\tinfo.head: " << info.head << endl
> << "\tinfo.offset: " << info.offset << endl;
> + cverb << vconvert << message;
> return 0;
> }
> }
> @@ -1396,9 +1412,12 @@ void OP_perf_utils::op_record_kernel_info(string vmlinux_file, u64 start_addr, u
> mmap.header.size = (sizeof(mmap) -
> (sizeof(mmap.filename) - size));
> int num = op_write_output(output_fd, &mmap, mmap.header.size);
> - if (cverb << vrecord)
> - cout << "Created MMAP event of size " << mmap.header.size << " for " <<mmap.filename << ". length: "
> + if (cverb << vrecord) {
> + ostringstream message;
> + message << "Created MMAP event of size " << mmap.header.size << " for " <<mmap.filename << ". length: "
> << hex << mmap.len << "; start addr: " << mmap.start << endl;
> + cout << message;
> + }
> pr->add_to_total(num);
> _record_module_info(output_fd, pr);
> }
> diff --git a/libutil++/bfd_support.cpp b/libutil++/bfd_support.cpp
> index 5c3a365..2157e5e 100644
> --- a/libutil++/bfd_support.cpp
> +++ b/libutil++/bfd_support.cpp
> @@ -27,6 +27,7 @@
> #include <cstring>
> #include <cassert>
> #include <iostream>
> +#include <sstream>
> #include <fstream>
> #include <sstream>
> #include <string>
> @@ -434,8 +435,10 @@ bool find_separate_debug_file(bfd * ibfd, string const & filepath_in,
> string second_try(DEBUGDIR + filedir + basename);
> string third_try(filedir + basename);
>
> - cverb << vbfd << "looking for debugging file " << basename
> + ostringstream message;
> + message << "looking for debugging file " << basename
> << " with crc32 = " << hex << crc32 << endl;
> + cverb << vbfd << message;
>
> if (separate_debug_file_exists(first_try, crc32, extra))
> debug_filename = first_try;
>
|