Re: Patch for ia64

[John Levon <levon@mo...>]

On Fri, Oct 31, 2003 at 06:20:44PM +0000, Philippe Elie wrote:

> >I don't agree that extern op_cpu is an abuse of a global variable, and I
> >also don't think it's positive to expose it publically rather than the
> >one or two places that actually need it.
> 
> you don't get the point, what is different between

To clarify: I agree in general, but not in this particular instance.

regards
john

-- 
Khendon's Law:
If the same point is made twice by the same person, the thread is over.

Re: Patch for ia64

[Philippe Elie <phil.el@wa...>]

John Levon wrote:
> On Fri, Oct 31, 2003 at 04:56:23PM +0000, Philippe Elie wrote:
> 
> 
>>in another source. That the third or fourth time I fix such
>>things in oprofile. if we pollute header with extern declaration
>>it means we abuse of global variable not than putting extern decl
>>in header is bad. (Note than extern int foo; is a declaration like
>>void bar(int);)
> 
> 
> I don't agree that extern op_cpu is an abuse of a global variable, and I
> also don't think it's positive to expose it publically rather than the
> one or two places that actually need it.

you don't get the point, what is different between

string basename(string const & path_name)
{
   extern string erase_to_last_of(string const &, char);
   ...
}

and an extern global var declaration ? both are declaration,
in both case we don't let the compiler the possiblity to check
for a mismatch.

 >>in another source. That the third or fourth time I fix such
                            ^^^^^^^^^^^^^^^^^^^

one of these day we will get a:

unsigned int * global_var;
and
extern unsigned long * global_var;

This will work on x86 but not on 64 bits arch (even worst
it can works on some 64 bits arch dependending on endianess
and the value stored in this var)

anyway no big deal, it's just IMHO a weirds habits to try
to hide to the compiler a part of the ABI (for me global
var are part of the ABI like global functions)

regards,
Phil

Re: Patch for ia64

[Will Cohen <wcohen@re...>]

John Levon wrote:
> On Fri, Oct 31, 2003 at 11:21:04AM -0500, Will Cohen wrote:
> 
> 
>>I was bit paranoid about removing the #ifndef.
> 
> 
> but you didn't just not remove the #ifndef, you *changed* it. See the
> current CVS sources for something that should hopefully work no matter
> what.

I am confused. The revise patch I sent in and CVS sources look identical 
around the area for the ifndef with the exception of the comment that 
autoconf should deal with it.

-Will

> What we really need, though, is a couple of autoconf tests to  see if
> they're in glibc, and if so, prefer the library implementation.
> 
> regards
> john
> 

Re: Patch for ia64

[John Levon <levon@mo...>]

On Fri, Oct 31, 2003 at 11:21:04AM -0500, Will Cohen wrote:

> I was bit paranoid about removing the #ifndef.

but you didn't just not remove the #ifndef, you *changed* it. See the
current CVS sources for something that should hopefully work no matter
what.

What we really need, though, is a couple of autoconf tests to  see if
they're in glibc, and if so, prefer the library implementation.

regards
john

-- 
Khendon's Law:
If the same point is made twice by the same person, the thread is over.

Re: Patch for ia64

[Will Cohen <wcohen@re...>]

John Levon wrote:
> On Fri, Oct 31, 2003 at 09:51:23AM -0500, Will Cohen wrote:
> 
> 
>>>>+#ifndef sched_setaffinity
>>>
>>>Seems dubious. When would it ever be a define ?
>>
>>I limited the ifndef to just the syscall number. In the RHEL3 kernel the 
>>number is defined and as a result the sched_setaffinity function wasn't 
>>in the code.
> 
> 
> You missed my point. When would there ever be a #define
> sched_setaffinity ? Never AFAICS. So that #ifndef is useless.

I was bit paranoid about removing the #ifndef. I though there could be a 
situation where /usr/include/asm/unistd.h doesn't have it (e.g. an old 
kernel header), but the actual kernel running on the machine does have 
the syscall. I didn't want to break things for other builds. However, I 
don't know when the sched_setaffinity syscall was added to the kernel.

>>>Please include this in the file(s) that actually need it.
>>
>>I revised the patch to put the '#include "op_cpu_type.h"' before the 
>>includes for oprofiled.h where required. Attached is the revised patch. 
>>It has been verified to apply cleanly and compile on ia64 and x86.
> 
> 
> Eek no. All that is needed is to put extern op_cpu cpu_type;  in
> opd_perfmon.c, plus the header - no need to pollute all the headers.
> 
> I'll boot up my IA64 box and do the necessary fixes ...
> 
> regards
> john

-Will

Re: Patch for ia64

[John Levon <levon@mo...>]

On Fri, Oct 31, 2003 at 04:56:23PM +0000, Philippe Elie wrote:

> in another source. That the third or fourth time I fix such
> things in oprofile. if we pollute header with extern declaration
> it means we abuse of global variable not than putting extern decl
> in header is bad. (Note than extern int foo; is a declaration like
> void bar(int);)

I don't agree that extern op_cpu is an abuse of a global variable, and I
also don't think it's positive to expose it publically rather than the
one or two places that actually need it.

regards
john
-- 
Khendon's Law:
If the same point is made twice by the same person, the thread is over.

Re: Patch for ia64

[Philippe Elie <phil.el@wa...>]

John Levon wrote:
> On Fri, Oct 31, 2003 at 09:51:23AM -0500, Will Cohen wrote:


>>>Please include this in the file(s) that actually need it.
>>
>>I revised the patch to put the '#include "op_cpu_type.h"' before the 
>>includes for oprofiled.h where required. Attached is the revised patch. 
>>It has been verified to apply cleanly and compile on ia64 and x86.
> 
> 
> Eek no. All that is needed is to put extern op_cpu cpu_type;  in
> opd_perfmon.c, plus the header - no need to pollute all the headers.

humm. I fixed a few days ago a problem with a declaration

unsigned int nr_images;

and a:

extern int nr_images;

in another source. That the third or fourth time I fix such
things in oprofile. if we pollute header with extern declaration
it means we abuse of global variable not than putting extern decl
in header is bad. (Note than extern int foo; is a declaration like
void bar(int);)


regards,
Phil

Re: Patch for ia64

[John Levon <levon@mo...>]

On Fri, Oct 31, 2003 at 09:51:23AM -0500, Will Cohen wrote:

> >>+#ifndef sched_setaffinity
> >
> >Seems dubious. When would it ever be a define ?
> 
> I limited the ifndef to just the syscall number. In the RHEL3 kernel the 
> number is defined and as a result the sched_setaffinity function wasn't 
> in the code.

You missed my point. When would there ever be a #define
sched_setaffinity ? Never AFAICS. So that #ifndef is useless.

> >Please include this in the file(s) that actually need it.
> 
> I revised the patch to put the '#include "op_cpu_type.h"' before the 
> includes for oprofiled.h where required. Attached is the revised patch. 
> It has been verified to apply cleanly and compile on ia64 and x86.

Eek no. All that is needed is to put extern op_cpu cpu_type;  in
opd_perfmon.c, plus the header - no need to pollute all the headers.

I'll boot up my IA64 box and do the necessary fixes ...

regards
john

-- 
Khendon's Law:
If the same point is made twice by the same person, the thread is over.

Re: Patch for ia64

[Will Cohen <wcohen@re...>]

John Levon wrote:
> On Thu, Oct 30, 2003 at 03:17:43PM -0500, Will Cohen wrote:
> 
> 
>>+#ifndef sched_setaffinity
> 
> 
> Seems dubious. When would it ever be a define ?

I limited the ifndef to just the syscall number. In the RHEL3 kernel the 
number is defined and as a result the sched_setaffinity function wasn't 
in the code.

>>-	memset(load_args, 0, sizeof(load_args));
>>+	memset(&load_args, 0, sizeof(load_args));
> 
> 
> Whoops.
> 
> 
>>+++ oprofile-0.7.1cvs-20031030/daemon/oprofiled.h	2003-10-30 12:34:42.996088651 -0500
>> 
>> #include <signal.h>
>>+#include "op_cpu_type.h"
> 
> 
> Please include this in the file(s) that actually need it.

I revised the patch to put the '#include "op_cpu_type.h"' before the 
includes for oprofiled.h where required. Attached is the revised patch. 
It has been verified to apply cleanly and compile on ia64 and x86.

2003-10-31  Will Cohen  <wcohen@...>

     * daemon/opd_perfmon.c: Split ifndef for sched_setaffinity.
       (load_context): Correct memset argument.
     * daemon/oprofiled.h: Declare cpu_type extern.
     * daemon/opd_perfmon.c:
     * daemon/opd_cookie.c:
     * daemon/opd_events.c:
     * daemon/opd_kernel.c:
     * daemon/opd_mangling.c:
     * daemon/opd_sfile.c:
     * daemon/init.c:
     * daemon/liblegacy/opd_image.c:
     * daemon/liblegacy/opd_kernel.c:
     * daemon/liblegacy/opd_proc.c:
     * daemon/liblegacy/opd_sample_files.c:
     * daemon/liblegacy/init.c: include op_cpu_type.h where required.

-Will

Re: Patch for ia64

[John Levon <levon@mo...>]

On Thu, Oct 30, 2003 at 03:17:43PM -0500, Will Cohen wrote:

> +#ifndef sched_setaffinity

Seems dubious. When would it ever be a define ?

> -	memset(load_args, 0, sizeof(load_args));
> +	memset(&load_args, 0, sizeof(load_args));

Whoops.

> +++ oprofile-0.7.1cvs-20031030/daemon/oprofiled.h	2003-10-30 12:34:42.996088651 -0500
>  
>  #include <signal.h>
> +#include "op_cpu_type.h"

Please include this in the file(s) that actually need it.

regards
john

-- 
Khendon's Law:
If the same point is made twice by the same person, the thread is over.