On Wed, Oct 15, 2008 at 04:32:42PM -0700, Jan Dubois wrote:
> On Sun, 12 Oct 2008, Tim Bunce wrote:
> > On Fri, Oct 10, 2008 at 01:17:21PM -0700, Jan Dubois wrote:
> > > Or is it preferable to keep everything internal to Devl::NYTProf and
> > > just extend the logic to deal with drive letters on Windows and
> > > normalize forward backward slashes at the I/O boundary?
> >
> > That sounds best, but I'm not clear about what needs changing, exactly.
> >
> > In get_file_id() the only obvious issue is the "determine absolute path
> > if file_name is relative" block. That probably ought to be rewritten
> > roughly like this:
> >
> > if (!found->eval_fid && is_filename_relative(file_name)) {
> > char *file_name_abs = get_absolute_filename(file_name);
> > if (file_name_abs)
> > found->key_abs = strdup(file_name_abs);
> > }
> >
> > The new is_filename_relative() and get_absolute_filename() functions
> > could encapsulate the portability code.
>
> I've just hardcoded the changes in the various places for now.
> Interestingly enough the absolute-filename code in get_file_id() doesn't
> seem to be exercised by the regression tests as I got the full suite to
> pass before I ported that part of the code...
It is exercised but wasn't tested for. The test code normalizes absolute
paths back to relative ones to avoid having specific paths in the tests.
I've fixed that now in r510.
> > What other portability issues are there?
>
> It was actually less than I thought at first. I'm attaching a patch
> that lets you build the module on Windows and also cleans up most of
> the compiler warnings in 64-bit mode.
Yeah! Many thanks Jan! Applied as r509.
> With the patch I can build against both 5.8.8 and 5.10.0 on Win32 with
> both the VC6 compiler and with GCC from MinGW. All tests are passing.
> It will also work with 64-bit Windows with the 64-bit C compiler from
> the Windows Platform SDK. I did have to disable the last 2 tests
> from 50.errno.t.
>
> Those 2 tests look bogus to me though, as there is no guarantee that
> errno is being preserved by *any* C runtime library call. errno is
> *only* valid after a failed runtime or system call:
>
> | The setting of errno after a successful call to a function is
> | unspecified unless the description of that function specifies that
> | errno shall not be modified.
>
> POSIX: http://www.opengroup.org/onlinepubs/009695399/functions/errno.html
>
> I did not change 50.errno.t, but I would recommend to remove the
> tests that expect errno to remain unchanged after a large amount
> of (unknown) code has been executed.
The problem is that it's important that NYTProf doesn't change errno
itself. That was causing some subtle bugs in code that was depending on
being able to test $!:
http://rt.cpan.org/Ticket/Display.html?id=39396
http://rt.cpan.org/Ticket/Display.html?id=39773
I've changed the test to avoid 'unknown' code: r511.
> I also tested the attached patch on a 64-bit Linux system, where it
> also passed all tests.
>
> I did not build with ZLIB support yet, so I don't know if I messed
> up anything in those conditional branches.
Seems fine on non-windows.
> Please review the patch carefully; I may have mixed in a few cases
> where I thought I was fixing a bug instead of just porting to Windows
> or getting rid of compiler warnings.
>
> Here are some additional comments:
>
> | Index: NYTProf.xs
> | ===================================================================
> | --- NYTProf.xs (revision 508)
> | +++ NYTProf.xs (working copy)
> | @@ -90,7 +90,7 @@
> |
> | #define NYTP_TAG_NO_TAG '\0' /* Used as a flag to mean "no tag"
> */
> |
> | -#define output_int(i) output_tag_int(NYTP_TAG_NO_TAG, (i))
> | +#define output_int(i) output_tag_int(NYTP_TAG_NO_TAG, (unsigned
> int)(i))
>
> output_tag_int() only works for 32 bit integers; I don't know if
> it can ever be called with 64-bit values in a 64-bit Perl.
Could you add some code that at least detects that situation
and issues a warning?
> | +/* XXX The proper return value would be Off_t */
> | static long
> | NYTP_tell(NYTP_file file) {
> | #ifdef HAS_ZLIB
>
> Decided not to bother using Off_t because the values are only used in error
> messages. If we use Off_t, then we would need to figure out which printf
> formatting sequence to use for it. :(
Fair enough.
> | croak("Profile format error whilst reading %s at %ld%s: expected
> %d got %d, %s",
> | - what, NYTP_tell(ifile), NYTP_type_of_offset(ifile), len,
> got,
> | + what, NYTP_tell(ifile), NYTP_type_of_offset(ifile),
> (int)len, (int)got,
> | (NYTP_eof(in)) ? "end of file" : NYTP_fstrerror(in));
>
> Casting size_t to int is kind of wrong, but again, what is the portable
> formatting code for size_t?
I've changed them to long to at least be (very slightly) better.
> | -static unsigned int
> | +static int
> | NYTP_printf(NYTP_file ofile, const char *format, ...) {
> | - unsigned int retval;
> | + int retval;
>
> POSIX defines the printf() return code as signed, negative values signaling
> an error condition.
Thanks. I've added error checking now.
> | @@ -973,7 +974,23 @@
> | output_int(fid_info->fid_flags);
> | output_int(fid_info->file_size);
> | output_int(fid_info->file_mtime);
> | - output_str(file_name, file_name_len);
> | +
> | +#ifdef WIN32
> | + /* Make sure we only use forward slashes in filenames */
> | + if (memchr(file_name, '\\', file_name_len)) {
> | + STRLEN i;
> | + char *file_name_copy = (char*)safemalloc(file_name_len);
> | + for (i=0; i<file_name_len; ++i) {
> | + char ch = file_name[i];
> | + file_name_copy[i] = ch == '\\' ? '/' : ch;
> | + }
> | + output_str(file_name_copy, (I32)file_name_len);
> | + Safefree(file_name_copy);
> | + return;
> | + }
> | +#endif
> | +
> | + output_str(file_name, (I32)file_name_len);
> | }
>
> This part is mostly needed to translate the "autosplit" names in the filename.
> I'm not sure this is really necessary for normal use of the module, but
> the regression tests contain sample results with hard-coded forward slashes.
>
> The temporary heap alloc also seems a bit wasteful, but modifying the strings
> in-place felt wrong, as I'm not really sure who "owns" them. They are just
> pointers into COP data, right?
At that point the strings have been copied into allocated memory by
hash_op(). But you can't edit the string in place or else later calls to
get_fild_id() won't find a matching file (key) so will create a new entry.
Since the performance of get_fild_id is critical the file name can't be
checked for backslashes there. So doing it in emit_fid() like you have
is fine. It's only called once per fid so the cost is low.
> | /* inserted new entry */
> | - if (1 == hash_op(entry, &found, created_via)) {
> | + if (1 == hash_op(entry, &found, (bool)created_via)) {
>
> Given that created_via contains flags, should this be written explicitly
> as (bool)(created_via != 0) ?
Done.
> | /* determine absolute path if file_name is relative */
> | found->key_abs = NULL;
> | - if (!found->eval_fid && *file_name != '/') {
> | + if (!found->eval_fid &&
> | +#ifdef WIN32
> | + /* XXX should we check for UNC names too? */
> | + (file_name_len < 3 || !isALPHA(file_name[0]) || file_name[1]
> != ':' ||
> | + (file_name[2] != '/' && file_name[2] != '\\'))
> | +#else
> | + *file_name != '/'
> | +#endif
> | + )
> | + {
> | - else if (strNE(file_name_abs, "/")) {
> | - if (strnEQ(file_name, "./", 2))
> | - ++file_name;
> | - else
> | - strcat(file_name_abs, "/");
> | + else {
> | +#ifdef WIN32
> | + char *p = file_name_abs;
> | + while (*p) {
> | + if ('\\' == *p)
> | + *p = '/';
> | + ++p;
> | + }
> | + if (p[-1] != '/') {
> | +#else
> | + if (strNE(file_name_abs, "/")) {
> | +#endif
> | + if (strnEQ(file_name, "./", 2))
> | + ++file_name;
> | + else
> | + strcat(file_name_abs, "/");
> | + }
> | strncat(file_name_abs, file_name, file_name_len);
> | found->key_abs = strdup(file_name_abs);
> | }
>
> None of the changes above were necessary to get the tests to pass, so
> I'm not too sure if this code is all correct or not.
Try it now I've updated the tests.
> Note that this change also modifies the Unix code path by moving the
> closing brace for the " if (strNE(file_name_abs, "/")) {" condition
> before the " strncat(file_name_abs, file_name, file_name_len);" line.
>
> To me it looks like the code was only supposed to prevent doubling
> the '/' if your current directory is the root, but you would always
> want to append the file_name to the cwd.
I recall some wierdness in relation to mod_perl when writing that code,
but I agree that sure looks like a bug fix. Thanks.
> | @@ -1694,7 +1732,7 @@
> | {
> | char filename_buf[MAXPATHLEN];
> |
> | - if (profile_opts & NYTP_OPTf_ADDPID
> | + if ((profile_opts & NYTP_OPTf_ADDPID)
> | || out /* already opened so assume forking */
> | ) {
> | sprintf(filename_buf, "%s.%d", filename, getpid());
>
> I just realized that this change wasn't necessary.
Not necessary, but no harm and probably good practice.
> | @@ -1704,7 +1742,7 @@
> |
> | /* some protection against multiple processes writing to the same file
> */
> | unlink(filename); /* throw away any previous file */
> | - out = NYTP_open(filename, "wbx");
> | + out = NYTP_open(filename, "wb");
> | if (!out) {
> | int fopen_errno = errno;
> | char *hint = "";
>
> I don't know what mode "wbx" is; the "x" is not allowed by POSIX.
It's not mentioned in the fopen() man page on OS X, nor a Linux
2.6.9-67.ELxenU system, but is in the man page for a Linux
2.6.18-53.1.21.el5 system:
GLIBC EXTENSIONS
The GNU C library allows the following extensions for the string
specified in mode:
x Open the file exclusively (like the O_EXCL flag of open(2)).
If the file already exists, fopen() fails, and sets errno to
EEXIST. This flag is ignored for fdopen().
Does having the 'x' present cause fopen() to fail on Windows?
> | - sc[NYTP_SCi_CALL_COUNT] = output_uv_from_av(aTHX_ av,
> NYTP_SCi_CALL_COUNT, 0);
> | + sc[NYTP_SCi_CALL_COUNT] = (NV)output_uv_from_av(aTHX_ av,
> NYTP_SCi_CALL_COUNT, 0);
> | - sc[NYTP_SCi_REC_DEPTH] = output_uv_from_av(aTHX_ av,
> NYTP_SCi_REC_DEPTH , 0);
> | + sc[NYTP_SCi_REC_DEPTH] = (NV)output_uv_from_av(aTHX_ av,
> NYTP_SCi_REC_DEPTH , 0);
>
> For some reason I get a warning from GCC in 64-bit mode for these 2 casts:
>
> NYTProf.xs:2532: warning: cast does not match function type
> NYTProf.xs:2538: warning: cast does not match function type
I'm not in 64-bit mode (intsize=4, longsize=4, doublesize=8)
and I get these (from gcc 4.0.1)
NYTProf.xs:2538: warning: cast from function call of type ‘UV’ to non-matching
type ‘double’
NYTProf.xs:2544: warning: cast from function call of type ‘UV’ to non-matching
type ‘double’
> Those warnings seem bogus though, and the casts are necessary for 64-bit mode
> where a UV cannot be converted into an NV without potential loss of precision.
I came up with a neat-ish fix:
- sc[NYTP_SCi_REC_DEPTH] = (NV)output_uv_from_av(aTHX_ av, NYTP_SCi_REC_DEPTH
, 0);
+ sc[NYTP_SCi_REC_DEPTH] = output_uv_from_av(aTHX_ av, NYTP_SCi_REC_DEPTH ,
0) * 1.0;
:-)
> ----------------------------------------------------------------------
>
> I think there are at least 2 more things that _should_ be done for
> Windows:
>
> 1) Use the high performance counters directly for time measurement.
> Using Time::HiRes is somewhat counter-productive, as we are using
> the high-preformance counters there, put periodically adjust the
> return value to incorporate any skew that the actual system clock
> may have (or if the user changed the system time). In the profiler
> we want the raw timings, which should also be a lot faster than
> going through all the conversions to turn them into the time of day
> values.
Agreed. I'd recommend you go the route of emulating clock_gettime().
Perhaps by extracting a chunk from
http://gnuwin32.sourceforge.net/packages/libgw32c.htm
see src/libgw32c/0.4/libgw32c-0.4/rt/clock_gettime.c file.
> 2) Windows should also use PERL_NO_GET_CONTEXT. This will require
> sprinkling more dTHX macros and/or passing aTHX_ around to all
> functions doing I/O (possibly a combination of both).
>
> But those changes will have to wait for another day...
Understood.
Thanks for your help Jan. Lots of people have been asking me for NYTProf
to support windows. I'm delighted you've made that happen.
Tim.
--~--~---------~--~----~------------~-------~--~----~
You've received this message because you are subscribed to
the Devel::NYTProf Development User group.
Group hosted at: http://groups.google.com/group/develnytprof-dev
Project hosted at: http://perl-devel-nytprof.googlecode.com
CPAN distribution: http://search.cpan.org/dist/Devel-NYTProf
To post, email: [email protected]
To unsubscribe, email: [EMAIL PROTECTED]
-~----------~----~----~----~------~----~------~--~---
