Answers to your remarks, but no patch file yet, I'm still testing my changes.
On Thu, 2006-12-28 at 21:57 +0000, Pedro Alves wrote:
> Not yet. Comments inline with your patch below. I've removed the
> autogenerated files for clarity. Please post the patches as text, not as
> tar.gz. After all, this is a -devel list :)
I reduced the size, because some mailing list mailers refuse large
messages.
> The '#if 1' above is one of those debugging statements?
Fixed.
> Watch out for the wproffile = &nlw[0] assignment!
Fixed.
> But, looking closelly, since that code, seems to be trying to get
> around the fact that WinCE has no cwd (current directory) support/notion,
> what about using GetModuleFileName to get at the directory where the exe is,
> and put gmon.out there? I mean always name it gmon.out.
Your solution is a less elegant one. Oh well, I followed your suggestion
and rewrote that piece.
> Leak. A FormatMessageW with FORMAT_MESSAGE_ALLOCATE_BUFFER should be
> matched with a LocalFree.
Fixed. I also added the missing MessageBoxW call.
> > @@ -1,4 +1,10 @@
> > #
> > +# This is a Makefile.in for very limited use : only create and install
> > +# the profiling runtime for cegcc (arm-wince-cegcc and
> > arm-wince-mingw32ce).
> > +#
> > +# This should NOT be fed back to the MingW project as is, it doesn't fit
> > +# nicely into their sources.
> > +#
>
> Yikes! Why are these changes needed? Ah, you used --target, instead of
> --host.
The answer is above : I adapted the src/mingw/profile build so it could
work as a standalone directory. The reason is that we need this stuff
for cegcc as well, and we can't install the src/mingw tree there.
Don't you remember that we chatted about that, and I said I'd implement
this as a standalone directory ? I asked where it should be (src/mingw
is not the right place for it) but we never made a decision on that.
> > + ${BASE_DIRECTORY}/mingw/profile/configure \
> > + --target=${TARGET} \
> > + --prefix=${PREFIX} \
> > + || exit
> > +
>
>
> Here. Use --host=${TARGET} here. The top level configure script
> takes care of the --target -> --host change, but you are not calling
> the top level one. (Top level is the configure found at gcc/configure,
> src/binutils/configure, etc. )
Unless I have to rework the whole src/mingw/profile configuration, I
don't think this is wrong.
> Put it before build_gdb instead, so people who can't build gdb currently
> (x64 guys), will still have profile support built. Oh, and the same for
> the build_docs call.
Ok.
> There's setvbuf in coredll.dll, use that instead.
ok.
> Why are you wrapping this? When using a static libgcc.a, nothing will call
> it, so it is as if it didn't exist. The only bad effect is that is grows
> cegcc.dll a bit, but that you took care on cegcc/Makefile. If there isn't
> a good reason, I would like to keep gcc global patches (not in config/arm)
> to a minimum. (Not a major thing to worry about, since we are still
> a young project, but do keep in mind that libgcc.a is supposed to be binary
> compatible forever. )
Ok, reverted that.
> Humm, why a host environment variable? Since you are implementing a
> host option, it would be better to have a command line switch
> instead of a transparent, environment variable, no?
I thought about that but this matches the approach for the other
variables, and it was easy to implement. Open for discussion.
> But, I think it would be better to leave the host part as it was (get rid of
> "GCOV_CROSS_PREFIX"), and just replace the getenv calls in mingw32ce version
> with WinCE Registry keys. In cegcc.dll the environ already comes from the
> registry. That way, the only thing special about WinCE profiling in
> comparison with other gcc targets, would be that you set a Registry key,
> instead of doing a shell export. Much more flexible, IMHO.
> I would go as far as implementing the a function that reads from
> the registry with the same interface as getenv, returning a pointer
> into a local static buffer, to keep our patch isolated, and call that
> instead of getenv.
>
> > +#ifdef UNDER_CE
> > + {
> > + wchar_t x[256];
> > + int l = strlen(gi_filename);
> > + l = (l < 256) ? l : 255;
> > + wcstombs(x, gi_filename, l);
> > + MessageBoxW(0, L"gcov_open", x, 0);
> > + }
> > +#endif
>
> This will go out, right?
Yes, that is one of them. You overlooked the "Yow" fprintf in gcc :-)
> s/UNDER_CE/__MINGW32CE__/ .
Fixed.
Danny
--
Danny Backx ; danny.backx - at - scarlet.be ; http://danny.backx.info
signature.asc
Description: This is a digitally signed message part
------------------------------------------------------------------------- Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________ Cegcc-devel mailing list Cegcc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/cegcc-devel
