On Mon, May 12, 2008 at 10:30 PM, Rafael Vanoni <Rafael.Vanoni at sun.com>
wrote:
>
> Aubrey Li wrote:
>
> > On Mon, May 12, 2008 at 6:26 PM, Rafael Vanoni <Rafael.Vanoni at sun.com>
> wrote:
> >
> > > Li, Aubrey wrote:
> > >
> > >
> > > > Rafael Vanoni wrote:
> > > >
> > > >
> > > >
> > > > > Aubrey Li wrote:
> > > > >
> > > > >
> > > > > > 2008/5/9 Rafael Vanoni Wrote:
> > > > > >
> > > > > >
> > > > > > > Here's a patch for this one.
> > > > > > > Simple case of using uint64_t or 32_t for one of the arguments
> to
> > > > > > > dtrace_lookup_by_addr.
> > > > > > >
> > > > > > >
> > > > > > I'm not sure you found the root cause.
> > > > > > There are a few dtrace_lookup_by_addr calls under
> > > > > > $(SRC)/lib/libdtrace/common, they are always using (uint64_t *) as
> > > > > > the pointer type. Any thoughts why?
> > > > > >
> > > > > >
> > > > > It's both type and casting. For instance, in
> > > > > http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/l
> > > > > ib/libdtrace/common/dt_consume.c#845
> > > > >
> > > > > There are references to dtrace_lookup_by_addr that always use
> > > > > uint64_t, and I'm not sure why they're not checking the bit depth.
> > > > > Could be either because it's still a private interface, the places
> > > > > that use it do the casting by themselves or it's a bug.
> > > > >
> > > > >
> > > > >
> > > > It is not a bug.
> > > > The address width should be 64-bit.
> > > >
> > > >
> > > Why ?
> > > The 32bit kernel will never have 64bit symbols, and vice-versa.
> > >
> > >
> >
> > I didn't mean the kernel symbols, I mean the parameter of
> > dtrace_lookup_by_addr().
> >
> > >
> > >
> > > > Our issue should be fixed like this:
> > > >
> > > > offender_addr = (void *)(data->dtada_data + rec1->dtrd_offset);
> > > > size = (64 or 32); /* sysinfo to obtain system information, 32-bit or
> 64
> > > > bit? */
> > > >
> > > > switch (size) {
> > > > case 32:
> > > > pc = *(uint32_t *)offender_addr;
> > > > break;
> > > > case 64:
> > > > pc = *(uint64_t *)offender_addr;
> > > > break;
> > > > }
> > > >
> > > > dtrace_lookup_by_addr(g_dtp, pc, &sym, &dts);
> > > > ...
> > > >
> > > >
> > > > Does this make sense?
> > > >
> > > >
> > > Well, both fixes are correct.
> > > I personally think there's no need for the extra code given that the
> > > #define __amd64 is there for exactly this purpose.
> > >
> > >
> >
> > "#define __amd64" forgot SPARC.
> >
> > And if powertop is taken out from ON makefile system,
> > there have to two different packages for i386 and amd64, even
> > if the difference is slight.
> >
>
> You're right, I overlook that.
> Here's the patch with these changes.
>
> thanks
> Rafael
It looks ok for me, :-)
please commit into powertop repo.
This should be the last changeset before we release powertop 1.0.
Thanks for your so quick so great work!
-Aubrey
>
>
> # HG changeset patch
> # User rafael.vanoni at sun.com
> # Date 1210602101 -3600
> # Node ID 84d9d7e6ecf8e4d611e3d5ac713fa8aa4fe916ca
> # Parent bd7ee06b95c5f79fc2ff04eea235ad2338a2b5e6
> Event report displays hex values on 32bit kernel
>
> diff -r bd7ee06b95c5 -r 84d9d7e6ecf8 usr/src/cmd/powertop/events.c
> --- a/usr/src/cmd/powertop/events.c Tue May 06 16:23:48 2008 +0100
> +++ b/usr/src/cmd/powertop/events.c Mon May 12 15:21:41 2008 +0100
> @@ -119,7 +119,7 @@
> dtrace_syminfo_t dts;
> char *offense_name;
> char *offender_name;
> - uint64_t *offender_addr;
> + uint64_t offender_addr;
> int32_t *instance;
> int i;
> uint64_t n = 0;
> @@ -140,23 +140,37 @@
>
> } else if (strcmp(aggdesc->dtagd_name, "events_k") == 0) {
>
> - offender_addr = (uint64_t *)(data->dtada_data +
> rec1->dtrd_offset);
> -
> snprintf((char *)(p_event->offender_name), EVENT_NAME_MAX,
> "%s", "<kernel>");
> +
> + /*
> + * Casting offender_addr to the wrong type will cause
> + * dtrace_lookup_by_addr to return 0 and the report
> + * the show an address instead of a name.
> + */
> + switch(bit_depth) {
> + case 32:
> + offender_addr = *(uint32_t *)(data->dtada_data +
> + rec1->dtrd_offset);;
> + break;
> + case 64:
> + offender_addr = *(uint64_t *)(data->dtada_data +
> + rec1->dtrd_offset);;
> + break;
> + }
>
> /*
> * We have the address of the kernel callout.
> * Try to resolve it into a meaningful symbol
> */
> - if (dtrace_lookup_by_addr(g_dtp, *offender_addr,
> + if (dtrace_lookup_by_addr(g_dtp, offender_addr,
> &sym, &dts) == 0) {
> snprintf((char *)(p_event->offense_name),
> EVENT_NAME_MAX, "%s`%s", dts.dts_object,
> dts.dts_name);
> } else {
> snprintf((char *)(p_event->offense_name),
> - EVENT_NAME_MAX, "0x%llx", *offender_addr);
> + EVENT_NAME_MAX, "0x%llx", offender_addr);
> }
>
> } else if (strcmp(aggdesc->dtagd_name, "events_u") == 0) {
> diff -r bd7ee06b95c5 -r 84d9d7e6ecf8 usr/src/cmd/powertop/powertop.c
> --- a/usr/src/cmd/powertop/powertop.c Tue May 06 16:23:48 2008 +0100
> +++ b/usr/src/cmd/powertop/powertop.c Mon May 12 15:21:41 2008 +0100
> @@ -79,6 +79,9 @@
> textdomain ("powertop");
>
> pt_set_progname(argv[0]);
> +
> + if ((bit_depth = get_bit_depth()) < 0)
> + return -1;
>
> ticktime = ticktime_usr = 5.0;
> displaytime = 0.0;
> diff -r bd7ee06b95c5 -r 84d9d7e6ecf8 usr/src/cmd/powertop/powertop.h
> --- a/usr/src/cmd/powertop/powertop.h Tue May 06 16:23:48 2008 +0100
> +++ b/usr/src/cmd/powertop/powertop.h Mon May 12 15:21:41 2008 +0100
> @@ -93,6 +93,11 @@
> char offense_name[EVENT_NAME_MAX];
> uint64_t total_count;
> } event_info_t;
> +
> +/*
> + * Stores the system's bit depth
> + */
> +int bit_depth;
>
> /*
> * Event accounting
> @@ -198,6 +203,7 @@
> extern void pt_set_progname(char *);
> extern void enumerate_cpus(void);
> extern void usage(void);
> +extern int get_bit_depth();
>
> /*
> * Display/curses related
> diff -r bd7ee06b95c5 -r 84d9d7e6ecf8 usr/src/cmd/powertop/util.c
> --- a/usr/src/cmd/powertop/util.c Tue May 06 16:23:48 2008 +0100
> +++ b/usr/src/cmd/powertop/util.c Mon May 12 15:21:41 2008 +0100
> @@ -37,6 +37,7 @@
> #include <sys/varargs.h>
> #include <sys/types.h>
> #include <sys/processor.h>
> +#include <sys/systeminfo.h>
>
> #include "powertop.h"
>
> @@ -97,3 +98,32 @@
> printf(" -h, --help Show this help message\n");
> exit(0);
> }
> +
> +int
> +get_bit_depth()
> +{
> + /*
> + * This little routine was derived from isainfo.c to look up
> + * the system's bit depth. It feeds a 10 byte long buffer to
> + * sysinfo (we only need the first word, sysinfo truncates and
> + * \0 terminates the rest) from which we figure out which isa
> + * we're running on.
> + */
> + size_t bufsize = 10;
> + char *buf;
> +
> + if ((buf = (char *)malloc(bufsize)) == NULL)
> + return (-1);
> +
> + if (sysinfo(SI_ARCHITECTURE_64, buf, bufsize) == -1)
> + if (sysinfo(SI_ARCHITECTURE_32, buf, bufsize) == -1)
> + return (-2);
> +
> + if (strcmp(buf, "sparc") == 0 || strcmp(buf, "i386") == 0)
> + return (32);
> +
> + if (strcmp(buf, "sparcv9") == 0 || strcmp(buf, "amd64") == 0)
> + return (64);
> +
> + return (-3);
> +}
>
>
