From: Maynard J. <may...@us...> - 2009-06-04 19:33:32
Attachments:
op-image-path.patch
|
I used 'oparchive -p /lib/modules/`uname -r`' to archive some profile data on a system, but when I brought the data over to another system to analyze, the '-p /lib/modules/`uname -r`' option didn't work for opreport. I got the errror "invalid --image-path= options". The attached patch prepends the archive path (if available) to the image path. As noted in the TODO, we still don't have a way of specifying separate image-paths for use with differential profiling, but this patch was an easy fix for a situation that is probably a lot more common than the differential profiling problem. -Maynard |
From: Maynard J. <may...@us...> - 2009-06-16 23:02:59
|
Suravee, Could you please review this patch? John, any comments from you would be appreciated as well. Thanks. -Maynard > I used 'oparchive -p /lib/modules/`uname -r`' to archive some profile > data on a system, but when I brought the data over to another system to > analyze, the '-p /lib/modules/`uname -r`' option didn't work for > opreport. I got the errror "invalid --image-path= options". The > attached patch prepends the archive path (if available) to the image > path. As noted in the TODO, we still don't have a way of specifying > separate image-paths for use with differential profiling, but this patch > was an easy fix for a situation that is probably a lot more common than > the differential profiling problem. > > -Maynard > > > > diff -paurX ../diff_fileExclusionFilter oprofile/ChangeLog op-image-path-fix/ChangeLog > --- oprofile/ChangeLog 2009-05-27 15:03:44.000000000 -0500 > +++ op-image-path-fix/ChangeLog 2009-06-04 13:49:03.000000000 -0500 > @@ -1,3 +1,9 @@ > +2009-06-04 Maynard Johnson <may...@us...> > + > + * libpp/profile_spec.cpp: > + * pp/common_option.cpp: Fix image-path option to be appended > + to archive path > + > 2009-05-27 Suravee Suthikulpanit <sur...@am...> > > * utils/opcontrol: Fix IBS initialization > diff -paurX ../diff_fileExclusionFilter oprofile/libpp/profile_spec.cpp op-image-path-fix/libpp/profile_spec.cpp > --- oprofile/libpp/profile_spec.cpp 2008-05-20 10:04:25.000000000 -0500 > +++ op-image-path-fix/libpp/profile_spec.cpp 2009-06-04 13:30:54.000000000 -0500 > @@ -326,6 +326,17 @@ profile_spec profile_spec::create(list<s > if (spec.session.empty()) > spec.session.push_back("current"); > > + bool ok = true; > + vector<string>::const_iterator ip_it = image_path.begin(); > + for ( ; ip_it != image_path.end(); ++ip_it) { > + if (!is_directory(spec.get_archive_path() + "/" + *ip_it)) { > + cerr << spec.get_archive_path() + "/" + *ip_it << " isn't a valid directory\n"; > + ok = false; > + } > + } > + if (!ok) > + throw op_runtime_error("invalid --image-path= options"); > + > spec.extra_found_images.populate(image_path, spec.get_archive_path(), > root_path); > vector<string>::const_iterator im = temp_image_or_lib.begin(); > diff -paurX ../diff_fileExclusionFilter oprofile/pp/common_option.cpp op-image-path-fix/pp/common_option.cpp > --- oprofile/pp/common_option.cpp 2008-04-29 07:07:46.000000000 -0500 > +++ op-image-path-fix/pp/common_option.cpp 2009-06-04 13:33:47.000000000 -0500 > @@ -180,18 +180,6 @@ options::spec get_options(int argc, char > exit(EXIT_FAILURE); > } > > - bool ok = true; > - vector<string>::const_iterator it = options::image_path.begin(); > - for ( ; it != options::image_path.end(); ++it) { > - if (!is_directory(*it)) { > - cerr << *it << " isn't a valid directory\n"; > - ok = false; > - } > - } > - > - if (!ok) > - throw op_runtime_error("invalid --image-path= options"); > - > // XML generator needs command line options for its header > ostringstream str; > for (int i = 1; i < argc; ++i) > > |
From: Suthikulpanit, S. <Sur...@am...> - 2009-06-22 21:59:00
|
Thank you for your description. I can now verified that the patch fixed the issue you mentioned. Suravee. -----Original Message----- From: Maynard Johnson [mailto:may...@us...] Sent: Monday, June 22, 2009 1:26 PM To: Suthikulpanit, Suravee Subject: Re: [PATCH] Fix --image-path option to work with an archive spec Suthikulpanit, Suravee wrote: > Maynard, > > I cannot reproduce the issue to verify the fix. Could you provide me > with some more pointers on how to generate the error message so that I > can verify it. Suravee, Here's a simple way to reproduce the error on one system: 1. cp -r /lib/modules/`uname -r` /tmp/myModules 2. 'opreport -p /tmp/myModules -l' should work OK. Verify that you can see symbols for samples in modules. 3. 'oparchive -p /tmp/myModules -o /tmp/myarchive' 4. Delete or rename /tmp/myModules. 5. 'opreport -p /tmp/myModules -l archive:/tmp/myarchive' should fail with "invalid --image-path= options". 6. Apply patch, build, and install. Re-run #5 above, and it should work OK. Thanks. -Maynard > > Suravee > > -----Original Message----- > From: Maynard Johnson [mailto:may...@us...] > Sent: Tuesday, June 16, 2009 6:03 PM > To: John Levon; Suthikulpanit, Suravee > Cc: opr...@li... > Subject: Re: [PATCH] Fix --image-path option to work with an archive > spec > > Suravee, > Could you please review this patch? > > John, any comments from you would be appreciated as well. > > Thanks. > -Maynard > >> I used 'oparchive -p /lib/modules/`uname -r`' to archive some profile >> data on a system, but when I brought the data over to another system > to >> analyze, the '-p /lib/modules/`uname -r`' option didn't work for >> opreport. I got the errror "invalid --image-path= options". The >> attached patch prepends the archive path (if available) to the image >> path. As noted in the TODO, we still don't have a way of specifying >> separate image-paths for use with differential profiling, but this > patch >> was an easy fix for a situation that is probably a lot more common > than >> the differential profiling problem. >> >> -Maynard >> >> >> >> diff -paurX ../diff_fileExclusionFilter oprofile/ChangeLog > op-image-path-fix/ChangeLog >> --- oprofile/ChangeLog 2009-05-27 15:03:44.000000000 -0500 >> +++ op-image-path-fix/ChangeLog 2009-06-04 13:49:03.000000000 > -0500 >> @@ -1,3 +1,9 @@ >> +2009-06-04 Maynard Johnson <may...@us...> >> + >> + * libpp/profile_spec.cpp: >> + * pp/common_option.cpp: Fix image-path option to be appended >> + to archive path >> + >> 2009-05-27 Suravee Suthikulpanit <sur...@am...> >> >> * utils/opcontrol: Fix IBS initialization >> diff -paurX ../diff_fileExclusionFilter > oprofile/libpp/profile_spec.cpp op-image-path-fix/libpp/profile_spec.cpp >> --- oprofile/libpp/profile_spec.cpp 2008-05-20 10:04:25.000000000 > -0500 >> +++ op-image-path-fix/libpp/profile_spec.cpp 2009-06-04 > 13:30:54.000000000 -0500 >> @@ -326,6 +326,17 @@ profile_spec profile_spec::create(list<s >> if (spec.session.empty()) >> spec.session.push_back("current"); >> >> + bool ok = true; >> + vector<string>::const_iterator ip_it = image_path.begin(); >> + for ( ; ip_it != image_path.end(); ++ip_it) { >> + if (!is_directory(spec.get_archive_path() + "/" + > *ip_it)) { >> + cerr << spec.get_archive_path() + "/" + *ip_it > << " isn't a valid directory\n"; >> + ok = false; >> + } >> + } >> + if (!ok) >> + throw op_runtime_error("invalid --image-path= options"); >> + >> spec.extra_found_images.populate(image_path, > spec.get_archive_path(), >> root_path); >> vector<string>::const_iterator im = temp_image_or_lib.begin(); >> diff -paurX ../diff_fileExclusionFilter oprofile/pp/common_option.cpp > op-image-path-fix/pp/common_option.cpp >> --- oprofile/pp/common_option.cpp 2008-04-29 07:07:46.000000000 > -0500 >> +++ op-image-path-fix/pp/common_option.cpp 2009-06-04 > 13:33:47.000000000 -0500 >> @@ -180,18 +180,6 @@ options::spec get_options(int argc, char >> exit(EXIT_FAILURE); >> } >> >> - bool ok = true; >> - vector<string>::const_iterator it = options::image_path.begin(); >> - for ( ; it != options::image_path.end(); ++it) { >> - if (!is_directory(*it)) { >> - cerr << *it << " isn't a valid directory\n"; >> - ok = false; >> - } >> - } >> - >> - if (!ok) >> - throw op_runtime_error("invalid --image-path= options"); >> - >> // XML generator needs command line options for its header >> ostringstream str; >> for (int i = 1; i < argc; ++i) >> >> > > > > |
From: Maynard J. <may...@us...> - 2009-06-23 15:06:21
|
Suthikulpanit, Suravee wrote: > Thank you for your description. I can now verified that the patch fixed > the issue you mentioned. Thanks. Patch committed. -Maynard > > Suravee. > > -----Original Message----- > From: Maynard Johnson [mailto:may...@us...] > Sent: Monday, June 22, 2009 1:26 PM > To: Suthikulpanit, Suravee > Subject: Re: [PATCH] Fix --image-path option to work with an archive > spec > > Suthikulpanit, Suravee wrote: >> Maynard, >> >> I cannot reproduce the issue to verify the fix. Could you provide me >> with some more pointers on how to generate the error message so that I >> can verify it. > Suravee, > Here's a simple way to reproduce the error on one system: > > 1. cp -r /lib/modules/`uname -r` /tmp/myModules > 2. 'opreport -p /tmp/myModules -l' should work OK. Verify that you can > see > symbols for samples in modules. > 3. 'oparchive -p /tmp/myModules -o /tmp/myarchive' > 4. Delete or rename /tmp/myModules. > 5. 'opreport -p /tmp/myModules -l archive:/tmp/myarchive' should fail > with > "invalid --image-path= options". > 6. Apply patch, build, and install. Re-run #5 above, and it should work > OK. > > Thanks. > -Maynard >> Suravee >> >> -----Original Message----- >> From: Maynard Johnson [mailto:may...@us...] >> Sent: Tuesday, June 16, 2009 6:03 PM >> To: John Levon; Suthikulpanit, Suravee >> Cc: opr...@li... >> Subject: Re: [PATCH] Fix --image-path option to work with an archive >> spec >> >> Suravee, >> Could you please review this patch? >> >> John, any comments from you would be appreciated as well. >> >> Thanks. >> -Maynard >> >>> I used 'oparchive -p /lib/modules/`uname -r`' to archive some profile > >>> data on a system, but when I brought the data over to another system >> to >>> analyze, the '-p /lib/modules/`uname -r`' option didn't work for >>> opreport. I got the errror "invalid --image-path= options". The >>> attached patch prepends the archive path (if available) to the image >>> path. As noted in the TODO, we still don't have a way of specifying >>> separate image-paths for use with differential profiling, but this >> patch >>> was an easy fix for a situation that is probably a lot more common >> than >>> the differential profiling problem. >>> >>> -Maynard >>> >>> >>> >>> diff -paurX ../diff_fileExclusionFilter oprofile/ChangeLog >> op-image-path-fix/ChangeLog >>> --- oprofile/ChangeLog 2009-05-27 15:03:44.000000000 -0500 >>> +++ op-image-path-fix/ChangeLog 2009-06-04 13:49:03.000000000 >> -0500 >>> @@ -1,3 +1,9 @@ >>> +2009-06-04 Maynard Johnson <may...@us...> >>> + >>> + * libpp/profile_spec.cpp: >>> + * pp/common_option.cpp: Fix image-path option to be appended >>> + to archive path >>> + >>> 2009-05-27 Suravee Suthikulpanit <sur...@am...> >>> >>> * utils/opcontrol: Fix IBS initialization >>> diff -paurX ../diff_fileExclusionFilter >> oprofile/libpp/profile_spec.cpp > op-image-path-fix/libpp/profile_spec.cpp >>> --- oprofile/libpp/profile_spec.cpp 2008-05-20 10:04:25.000000000 >> -0500 >>> +++ op-image-path-fix/libpp/profile_spec.cpp 2009-06-04 >> 13:30:54.000000000 -0500 >>> @@ -326,6 +326,17 @@ profile_spec profile_spec::create(list<s >>> if (spec.session.empty()) >>> spec.session.push_back("current"); >>> >>> + bool ok = true; >>> + vector<string>::const_iterator ip_it = image_path.begin(); >>> + for ( ; ip_it != image_path.end(); ++ip_it) { >>> + if (!is_directory(spec.get_archive_path() + "/" + >> *ip_it)) { >>> + cerr << spec.get_archive_path() + "/" + *ip_it >> << " isn't a valid directory\n"; >>> + ok = false; >>> + } >>> + } >>> + if (!ok) >>> + throw op_runtime_error("invalid --image-path= options"); >>> + >>> spec.extra_found_images.populate(image_path, >> spec.get_archive_path(), >>> root_path); >>> vector<string>::const_iterator im = temp_image_or_lib.begin(); >>> diff -paurX ../diff_fileExclusionFilter oprofile/pp/common_option.cpp >> op-image-path-fix/pp/common_option.cpp >>> --- oprofile/pp/common_option.cpp 2008-04-29 07:07:46.000000000 >> -0500 >>> +++ op-image-path-fix/pp/common_option.cpp 2009-06-04 >> 13:33:47.000000000 -0500 >>> @@ -180,18 +180,6 @@ options::spec get_options(int argc, char >>> exit(EXIT_FAILURE); >>> } >>> >>> - bool ok = true; >>> - vector<string>::const_iterator it = options::image_path.begin(); >>> - for ( ; it != options::image_path.end(); ++it) { >>> - if (!is_directory(*it)) { >>> - cerr << *it << " isn't a valid directory\n"; >>> - ok = false; >>> - } >>> - } >>> - >>> - if (!ok) >>> - throw op_runtime_error("invalid --image-path= options"); >>> - >>> // XML generator needs command line options for its header >>> ostringstream str; >>> for (int i = 1; i < argc; ++i) >>> >>> >> >> >> > > > |