On 6/4/2013 11:26 AM, Maynard Johnson wrote:
On 06/03/2013 04:54 PM, suravee.suthikulpanit@amd.com wrote:
From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Current libop does not allow named mask to be used as default mask.
This causes the tools to fail when user does not specify the named
mask of an event and relying on the numerical default unit mask
which could have duplication.
Hi, Suravee,
Thank you for this patch. This appears to be a good basis for solving the problem of named default unit masks. I need to test it on a Sandybridge that I can get access to tomorrow.  One comment I have now is this ... In parse_um_entry, is there a reason to set entry->name unconditionally?  Why not set it only when "extra" is non-zero? I don't think it makes any functional difference, but it just seems more logically correct.
Thanks. This make sense, I'll send V2 with the fix.
And just to be clear, the only thing we need from *Andi* to make this patch work correctly is to have the unit mask files that contain named UMs to be modified to use default names versus hex values where the value is ambiguous, right?
This patch should replace the patch 3/3 in the patch series 

[PATCH 1/3] Add EXTRA_NONE
http://marc.info/?l=oprofile-list&m=136279274904113&w=2

[PATCH 2/3] Add empty extra: line to unique Intel events v2
http://marc.info/?l=oprofile-list&m=136279158903886&w=2
[PATCH 3/3] Don't do uniqueness checking for default masks
http://marc.info/?l=oprofile-list&m=136279276004115&w=2

We will need Andi to make changes to Intel events with named masks which the default mask has duplication,
and use the proper named mask instead.

Suravee

Thanks!
-Maynard
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 libop/op_events.c |   39 +++++++++++++++++++++++++++++++++------
 libop/op_events.h |    2 ++
 2 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/libop/op_events.c b/libop/op_events.c
index 276386d..a35f370 100644
--- a/libop/op_events.c
+++ b/libop/op_events.c
@@ -197,7 +197,12 @@ static void parse_um(struct op_unit_mask * um, char const * line)
 			if (seen_default)
 				parse_error("duplicate default: tag");
 			seen_default = 1;
-			um->default_mask = parse_hex(tagend);
+			if (0 != strncmp(tagend, "0x", 2)) {
+				um->default_mask_name = op_xstrndup(
+					tagend, valueend - tagend);
+			} else {
+				um->default_mask = parse_hex(tagend);
+			}
 		} else {
 			parse_error("invalid unit mask tag");
 		}
@@ -240,6 +245,12 @@ static void parse_um_entry(struct op_described_um * entry, char const * line)
 	if (!*c)
 		parse_error("invalid unit mask entry");

+	// This is for the named_mask
+	entry->name= op_xstrndup(c, strcspn(c, " \t"));
+
+	c = skip_ws(c);
+
+	// This is for the desc
 	entry->desc = xstrdup(c);
 }

@@ -605,10 +616,17 @@ static int check_unit_mask(struct op_unit_mask const * um,
 				"(%s)\n", um->name, cpu_name);
 			err = EXIT_FAILURE;
 		}
-	} else {
-		for (i = 0; i < um->num; ++i) {
-			if (um->default_mask == um->um[i].value)
-				break;
+	} else if (um->unit_type_mask == utm_exclusive) {
+		if (um->default_mask_name) {
+			for (i = 0; i < um->num; ++i) {
+				if (0 == strcmp(um->default_mask_name, um->um[i].name))
+					break;
+			}
+		} else {
+			for (i = 0; i < um->num; ++i) {
+				if (um->default_mask == um->um[i].value)
+					break;
+			}
 		}

 		if (i == um->num) {
@@ -708,6 +726,7 @@ static void load_events(op_cpu cpu_type)
 	unit_mask->unit_type_mask = utm_mandatory;
 	unit_mask->um[0].extra = 0;
 	unit_mask->um[0].value = 0;
+	unit_mask->um[0].name = xstrdup("");
 	unit_mask->um[0].desc = xstrdup("No unit mask");
 	unit_mask->used = 1;

@@ -1335,10 +1354,18 @@ static void extra_check(struct op_event *e, char *name, unsigned w)
 	}
 }

-static void do_resolve_unit_mask(struct op_event *e, struct parsed_event *pe, u32 *extra)
+static void do_resolve_unit_mask(struct op_event *e,
+	struct parsed_event *pe, u32 *extra)
 {
 	unsigned i;

+	/* If not specified um and the default um is name type
+	 * we populate pe unitmask name with default name */
+	if ((e->unit->default_mask_name != NULL) &&
+		(pe->unit_mask_name == NULL)) {
+		pe->unit_mask_name = xstrdup(e->unit->default_mask_name);
+	}
+
 	for (;;) {
 		if (pe->unit_mask_name == NULL) {
 			/* For numerical unit mask */
diff --git a/libop/op_events.h b/libop/op_events.h
index a437f2a..33114f7 100644
--- a/libop/op_events.h
+++ b/libop/op_events.h
@@ -56,9 +56,11 @@ struct op_unit_mask {
 	u32 num;		/**< number of possible unit masks */
 	enum unit_mask_type unit_type_mask;
 	u32 default_mask;	/**< only the gui use it */
+	char * default_mask_name;
 	struct op_described_um {
 	        u32 extra;
 		u32 value;
+		char * name;
 		char * desc;
 	} um[MAX_UNIT_MASK];
 	struct list_head um_next; /**< next um in list */