Hey Stephane,
Just wondering why we used the type casting of (uint64_t *) on a data
which is defined as "int" in the structure of "perf_record_opts".
struct perf_record_opts {
struct perf_target target;
bool call_graph;
bool group;
bool inherit_stat;
bool no_delay;
bool no_inherit;
bool no_samples;
bool pipe_output;
bool raw_samples;
bool sample_address;
bool sample_time;
bool sample_id_all_missing;
bool exclude_guest_missing;
bool period;
unsigned int freq;
unsigned int mmap_pages;
unsigned int user_freq;
int branch_stack;
u64 default_interval;
u64 user_interval;
};
static int
parse_branch_stack(const struct option *opt, const char *str, int unset)
{
#define ONLY_PLM \
(PERF_SAMPLE_BRANCH_USER |\
PERF_SAMPLE_BRANCH_KERNEL |\
PERF_SAMPLE_BRANCH_HV)
uint64_t *mode = (uint64_t *)opt->value;
--
Regards
Anshuman Khandual
This code is breaking in powerpc systems.
(1) 'opt->value' gets updated inside the function parse_branch_stack via
dereferencing a (uint64_t *) type casted pointer.
(2) But the value is not accessible when we again use opt->value via
dereferencing a (int *) type casted pointer.
(3) As a result record.opts.branch_stack remains 0 and unchanged by parse_branch_stack
This is caused by bit representation of 'uint64_t' and 'int' in powerpc systems. Bytes update
for the data (when accessed trough (uint64_t *) casting) is no longer available to the
data when accessed through (int *) type casting. Verified this from bit representation of
the data (accessed through both type casting methods).
However this problem does not seem to be present on an Intel box. Integer dereferencing of
the opt->value still gives the value which was updated as (uint64_t).
All this problem would not have been there if we had used (int *) instead of (uint64_t *) in
the first place inside parse_branch_stack function.
On Thursday 24 May 2012 02:51 PM, Anshuman Khandual wrote:
> Hey Stephane,
>
> Just wondering why we used the type casting of (uint64_t *) on a data
> which is defined as "int" in the structure of "perf_record_opts".
>
> struct perf_record_opts {
> struct perf_target target;
> bool call_graph;
> bool group;
> bool inherit_stat;
> bool no_delay;
> bool no_inherit;
> bool no_samples;
> bool pipe_output;
> bool raw_samples;
> bool sample_address;
> bool sample_time;
> bool sample_id_all_missing;
> bool exclude_guest_missing;
> bool period;
> unsigned int freq;
> unsigned int mmap_pages;
> unsigned int user_freq;
> int branch_stack;
> u64 default_interval;
> u64 user_interval;
> };
>
> static int
> parse_branch_stack(const struct option *opt, const char *str, int unset)
> {
> #define ONLY_PLM \
> (PERF_SAMPLE_BRANCH_USER |\
> PERF_SAMPLE_BRANCH_KERNEL |\
> PERF_SAMPLE_BRANCH_HV)
>
> uint64_t *mode = (uint64_t *)opt->value;
> --
> Regards
> Anshuman Khandual
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On Fri, May 25, 2012 at 7:27 AM, Anshuman Khandual
<[email protected]> wrote:
> This code is breaking in powerpc systems.
>
> (1) 'opt->value' gets updated inside the function parse_branch_stack via
> dereferencing a (uint64_t *) type casted pointer.
>
> (2) But the value is not accessible when we again use opt->value via
> dereferencing a (int *) type casted pointer.
>
> (3) As a result record.opts.branch_stack remains 0 and unchanged by parse_branch_stack
>
> This is caused by bit representation of 'uint64_t' and 'int' in powerpc systems. Bytes update
> for the data (when accessed trough (uint64_t *) casting) is no longer available to the
> data when accessed through (int *) type casting. Verified this from bit representation of
> the data (accessed through both type casting methods).
>
> However this problem does not seem to be present on an Intel box. Integer dereferencing of
> the opt->value still gives the value which was updated as (uint64_t).
>
> All this problem would not have been there if we had used (int *) instead of (uint64_t *) in
> the first place inside parse_branch_stack function.
>
The bug is that in struct record_opts, branch_stack is declared int
instead of u64.
I can post a patch to fix that. The value is eventually passed to struct
perf_event_attr.branch_sample_type which is defined as u64.
I can post a patch to fix that.
Thanks for catching this.
> On Thursday 24 May 2012 02:51 PM, Anshuman Khandual wrote:
>
>> Hey Stephane,
>>
>> Just wondering why we used the type casting of (uint64_t *) on a data
>> which is defined as "int" in the structure of "perf_record_opts".
>>
>> struct perf_record_opts {
>> struct perf_target target;
>> bool call_graph;
>> bool group;
>> bool inherit_stat;
>> bool no_delay;
>> bool no_inherit;
>> bool no_samples;
>> bool pipe_output;
>> bool raw_samples;
>> bool sample_address;
>> bool sample_time;
>> bool sample_id_all_missing;
>> bool exclude_guest_missing;
>> bool period;
>> unsigned int freq;
>> unsigned int mmap_pages;
>> unsigned int user_freq;
>> int branch_stack;
>> u64 default_interval;
>> u64 user_interval;
>> };
>>
>> static int
>> parse_branch_stack(const struct option *opt, const char *str, int unset)
>> {
>> #define ONLY_PLM \
>> (PERF_SAMPLE_BRANCH_USER |\
>> PERF_SAMPLE_BRANCH_KERNEL |\
>> PERF_SAMPLE_BRANCH_HV)
>>
>> uint64_t *mode = (uint64_t *)opt->value;
>> --
>> Regards
>> Anshuman Khandual
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>>
>
>
Hi,
It should be something like that instead:
diff --git a/tools/perf/perf.h b/tools/perf/perf.h
index 8a9687e..c9ca7c4 100644
--- a/tools/perf/perf.h
+++ b/tools/perf/perf.h
@@ -228,7 +228,7 @@ struct perf_record_opts {
unsigned int freq;
unsigned int mmap_pages;
unsigned int user_freq;
- int branch_stack;
+ u64 branch_stack;
u64 default_interval;
u64 user_interval;
};
On Fri, May 25, 2012 at 10:03 AM, Anshuman Khandual
<[email protected]> wrote:
> perf record: Fixing record option data type in parse_branch_stack
>
> Currently parse_branch_stack does not update record.opts.branch_stack
> value in powerpc architecture. opt->value is declared as int in struct
> perf_record_opts. But is worked on as uint64_t isnide the function.
> This breaks functionality in poweprc due to bit representation
> of uint64_t which is inaccessible as int.
>
> Signed-off-by: Anshuman Khandual <[email protected]>
> ---
> tools/perf/builtin-record.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index e5cb084..161c0f1 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -677,7 +677,7 @@ parse_branch_stack(const struct option *opt, const char *str, int unset)
> PERF_SAMPLE_BRANCH_KERNEL |\
> PERF_SAMPLE_BRANCH_HV)
>
> - uint64_t *mode = (uint64_t *)opt->value;
> + int *mode = (int *)opt->value;
> const struct branch_mode *br;
> char *s, *os = NULL, *p;
> int ret = -1;
> --
> 1.7.9.5
>
>
>
> On Friday 25 May 2012 10:57 AM, Anshuman Khandual wrote:
>
>> This code is breaking in powerpc systems.
>>
>> (1) 'opt->value' gets updated inside the function parse_branch_stack via
>> dereferencing a (uint64_t *) type casted pointer.
>>
>> (2) But the value is not accessible when we again use opt->value via
>> dereferencing a (int *) type casted pointer.
>>
>> (3) As a result record.opts.branch_stack remains 0 and unchanged by parse_branch_stack
>>
>> This is caused by bit representation of 'uint64_t' and 'int' in powerpc systems. Bytes update
>> for the data (when accessed trough (uint64_t *) casting) is no longer available to the
>> data when accessed through (int *) type casting. Verified this from bit representation of
>> the data (accessed through both type casting methods).
>>
>> However this problem does not seem to be present on an Intel box. Integer dereferencing of
>> the opt->value still gives the value which was updated as (uint64_t).
>>
>> All this problem would not have been there if we had used (int *) instead of (uint64_t *) in
>> the first place inside parse_branch_stack function.
>>
>> On Thursday 24 May 2012 02:51 PM, Anshuman Khandual wrote:
>>
>>> Hey Stephane,
>>>
>>> Just wondering why we used the type casting of (uint64_t *) on a data
>>> which is defined as "int" in the structure of "perf_record_opts".
>>>
>>> struct perf_record_opts {
>>> struct perf_target target;
>>> bool call_graph;
>>> bool group;
>>> bool inherit_stat;
>>> bool no_delay;
>>> bool no_inherit;
>>> bool no_samples;
>>> bool pipe_output;
>>> bool raw_samples;
>>> bool sample_address;
>>> bool sample_time;
>>> bool sample_id_all_missing;
>>> bool exclude_guest_missing;
>>> bool period;
>>> unsigned int freq;
>>> unsigned int mmap_pages;
>>> unsigned int user_freq;
>>> int branch_stack;
>>> u64 default_interval;
>>> u64 user_interval;
>>> };
>>>
>>> static int
>>> parse_branch_stack(const struct option *opt, const char *str, int unset)
>>> {
>>> #define ONLY_PLM \
>>> (PERF_SAMPLE_BRANCH_USER |\
>>> PERF_SAMPLE_BRANCH_KERNEL |\
>>> PERF_SAMPLE_BRANCH_HV)
>>>
>>> uint64_t *mode = (uint64_t *)opt->value;
>>> --
>>> Regards
>>> Anshuman Khandual
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at http://www.tux.org/lkml/
>>>
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>>
>
>
perf record: Fixing record option data type in parse_branch_stack
Currently parse_branch_stack does not update record.opts.branch_stack
value in powerpc architecture. opt->value is declared as int in struct
perf_record_opts. But is worked on as uint64_t isnide the function.
This breaks functionality in poweprc due to bit representation
of uint64_t which is inaccessible as int.
Signed-off-by: Anshuman Khandual <[email protected]>
---
tools/perf/builtin-record.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index e5cb084..161c0f1 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -677,7 +677,7 @@ parse_branch_stack(const struct option *opt, const char *str, int unset)
PERF_SAMPLE_BRANCH_KERNEL |\
PERF_SAMPLE_BRANCH_HV)
- uint64_t *mode = (uint64_t *)opt->value;
+ int *mode = (int *)opt->value;
const struct branch_mode *br;
char *s, *os = NULL, *p;
int ret = -1;
--
1.7.9.5
On Friday 25 May 2012 10:57 AM, Anshuman Khandual wrote:
> This code is breaking in powerpc systems.
>
> (1) 'opt->value' gets updated inside the function parse_branch_stack via
> dereferencing a (uint64_t *) type casted pointer.
>
> (2) But the value is not accessible when we again use opt->value via
> dereferencing a (int *) type casted pointer.
>
> (3) As a result record.opts.branch_stack remains 0 and unchanged by parse_branch_stack
>
> This is caused by bit representation of 'uint64_t' and 'int' in powerpc systems. Bytes update
> for the data (when accessed trough (uint64_t *) casting) is no longer available to the
> data when accessed through (int *) type casting. Verified this from bit representation of
> the data (accessed through both type casting methods).
>
> However this problem does not seem to be present on an Intel box. Integer dereferencing of
> the opt->value still gives the value which was updated as (uint64_t).
>
> All this problem would not have been there if we had used (int *) instead of (uint64_t *) in
> the first place inside parse_branch_stack function.
>
> On Thursday 24 May 2012 02:51 PM, Anshuman Khandual wrote:
>
>> Hey Stephane,
>>
>> Just wondering why we used the type casting of (uint64_t *) on a data
>> which is defined as "int" in the structure of "perf_record_opts".
>>
>> struct perf_record_opts {
>> struct perf_target target;
>> bool call_graph;
>> bool group;
>> bool inherit_stat;
>> bool no_delay;
>> bool no_inherit;
>> bool no_samples;
>> bool pipe_output;
>> bool raw_samples;
>> bool sample_address;
>> bool sample_time;
>> bool sample_id_all_missing;
>> bool exclude_guest_missing;
>> bool period;
>> unsigned int freq;
>> unsigned int mmap_pages;
>> unsigned int user_freq;
>> int branch_stack;
>> u64 default_interval;
>> u64 user_interval;
>> };
>>
>> static int
>> parse_branch_stack(const struct option *opt, const char *str, int unset)
>> {
>> #define ONLY_PLM \
>> (PERF_SAMPLE_BRANCH_USER |\
>> PERF_SAMPLE_BRANCH_KERNEL |\
>> PERF_SAMPLE_BRANCH_HV)
>>
>> uint64_t *mode = (uint64_t *)opt->value;
>> --
>> Regards
>> Anshuman Khandual
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On Friday 25 May 2012 02:14 PM, Stephane Eranian wrote:
> Hi,
>
> It should be something like that instead:
>
> diff --git a/tools/perf/perf.h b/tools/perf/perf.h
> index 8a9687e..c9ca7c4 100644
> --- a/tools/perf/perf.h
> +++ b/tools/perf/perf.h
> @@ -228,7 +228,7 @@ struct perf_record_opts {
> unsigned int freq;
> unsigned int mmap_pages;
> unsigned int user_freq;
> - int branch_stack;
> + u64 branch_stack;
> u64 default_interval;
> u64 user_interval;
> };
>
Agreed. I just tried to fix the problem where it was more evident because
of the data type mismatch.
Em Fri, May 25, 2012 at 04:02:15PM +0530, Anshuman Khandual escreveu:
> On Friday 25 May 2012 02:14 PM, Stephane Eranian wrote:
>
> > Hi,
> >
> > It should be something like that instead:
> >
> > diff --git a/tools/perf/perf.h b/tools/perf/perf.h
> > index 8a9687e..c9ca7c4 100644
> > --- a/tools/perf/perf.h
> > +++ b/tools/perf/perf.h
> > @@ -228,7 +228,7 @@ struct perf_record_opts {
> > unsigned int freq;
> > unsigned int mmap_pages;
> > unsigned int user_freq;
> > - int branch_stack;
> > + u64 branch_stack;
> > u64 default_interval;
> > u64 user_interval;
> > };
> >
>
>
>
> Agreed. I just tried to fix the problem where it was more evident because
> of the data type mismatch.
Can you send an updated patch on a message just for that?
- Arnaldo