Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 360490

Summary: Implementation of method that obtains unit mask type does so incorrectly, but could more easily be implemented with "ophelp -X"
Product: z_Archived Reporter: Corey Ashford <cjashfor>
Component: LinuxToolsAssignee: Roland Grunberg <rgrunber>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: kksebasti, wainersm
Version: unspecified   
Target Milestone: ---   
Hardware: PC   
OS: Linux   
Whiteboard:
Attachments:
Description Flags
Patch none

Description Corey Ashford CLA 2011-10-10 21:07:05 EDT 
Build Identifier: M20110909-1335

The method org.eclipse.linuxtools.oprofile.core.opxml.EventIdCache.getUnitMaskType(String)
 reads /usr/share/oprofile/<cpu_type>/events and /usr/share/oprofile/<cpu_type>/unit_masks instead of simply using the output from ophelp -X.

Furthermore, because of the approach that was taken, the code actually processes the files incorrectly because it doesn't pay any attention to the "include:" directives in the files it reads.  For example, on my x86_64 laptop,
if I read /usr/share/oprofile/i386/core_i7/unit_masks, the only contents of that
file is "include:i386/nehalem".  The above method doesn't go and read the included file, so it's doesn't find any unit masks.

The implementation should change to use "ophelp -X" because it will do all of that include processing for you, and you will be able to use standard XML parsing the get the needed information.

Reproducible: Always
Comment 1 Roland Grunberg CLA 2011-10-11 11:13:01 EDT 
This functionality made it into oprofile upstream as commit f3c4ab9d2a9bc3aa2236ee3763ad1a39ba190686 (I was aware of the issues surrounding unit mask types and wanted ophelp to provide this functionality). This is newin 0.9.7 but I think it's worth having as opposed to relying on the reading various files manually. Thanks for the bug report.
Comment 2 Corey Ashford CLA 2011-10-11 18:04:41 EDT 
(In reply to comment #1)
> This functionality made it into oprofile upstream as commit
> f3c4ab9d2a9bc3aa2236ee3763ad1a39ba190686 (I was aware of the issues surrounding
> unit mask types and wanted ophelp to provide this functionality). This is newin
> 0.9.7 but I think it's worth having as opposed to relying on the reading
> various files manually. Thanks for the bug report.

I talked with one of the OProfile maintainers today (Maynard Johnson - mpjohn@us.ibm.com), and as a result I think that it might be a good idea to switch to using the output of "ophelp -X" exclusively.  On older systems (e.g. SLES 10), ophelp -X will simply generate an error, because the -X option is not supported at all.  However, we should be able to detect this from inside of the plug-in and issue an error that the user needs to upgrade his oprofile to a newer version.  The maintainer says that the oprofile kernel ABI hasn't changed in a very long time, so you should be able to download the latest oprofile source from sourceforge and build it, producing new ophelp and opcontrol executables, which by default will install in /usr/local/bin.  Of course, this means that the location of the op* tools has to be configurable!

If plug-in runs ophelp -X and notes that the schemaversion is not 1.1 or higher, it can tell the user that they need a newer version of oprofile installed.  If the schemaversion is 1.1 or higher, you can expect that the category attribute will be present in the ophelp -X output.
Comment 3 Roland Grunberg CLA 2011-12-20 16:52:57 EST 
I have a proposed patch I'm testing out but I'm not sure sure about dropping the functionality that looks inside /usr/share/oprofile. I'd rather have that around for those using an older version of oprofile installed on the standard path.
Comment 4 Roland Grunberg CLA 2011-12-21 10:41:09 EST 
Created attachment 208691 [details]
Patch
Comment 5 Corey Ashford CLA 2011-12-21 13:33:27 EST 
This looks like a good solution to me.  Thanks!
Comment 6 Roland Grunberg CLA 2011-12-21 16:36:46 EST 
Committed as 	4b28814587e8faf1943acbb18e17a78508034d15.
Comment 7 Roland Grunberg CLA 2011-12-22 15:08:19 EST 
Resolving as fixed.