Danny Backx escreveu:
> Err, that latest commit wasn't to mingwex. There's also one or two
> debugging statements still in there. I'll make sure to remove those
> prior to commit. The rest of my message is still valid :-)
>
> Danny
>
> On Thu, 2006-12-28 at 12:11 +0100, Danny Backx wrote:
>> Guys,
>>
>> Here's the profiling support patch. I've taken it out of the branch and
>> put it on trunk, but I've not imported Pedro's latest change (to
>> mingw/mingwex) yet so this may slightly conflict there.
>>
>> Pedro, is this good enough for commit ?
>>
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 :)
> Index: mingw/profile/gmon.c
> ===================================================================
> --- mingw/profile/gmon.c (revision 835)
> +++ mingw/profile/gmon.c (working copy)
> @@ -70,9 +70,17 @@
> #define SCALE_1_TO_1 0x10000L
>
> #ifdef UNDER_CE
> -#define ERR(s) fwrite(s, sizeof(s), 1, stderr)
> +#include <windows.h>
> +#endif
> +
> +#if 1
> +# define ERR(s) write((int)fileno(stderr), s, sizeof(s))
> #else
The '#if 1' above is one of those debugging statements?
> @@ -247,22 +249,18 @@
> {
> #define NAME_LEN 128
> wchar_t nlw[NAME_LEN];
> - char nl[NAME_LEN];
> int len;
>
> len = GetModuleFileNameW(NULL, nlw, NAME_LEN);
> if (len == 0) {
> - proffile = "gmon.out";
> + wproffile = L"gmon.out";
> } else {
> - if (wcstombs(nl, nlw, len) < 0) {
> - proffile = "gmon.out";
> - } else {
> - nl[len-3] = 'g';
> - nl[len-2] = 'm';
> - nl[len-1] = 'o';
> + nlw[len-3] = L'g';
> + nlw[len-2] = L'm';
> + nlw[len-1] = L'o';
> + nlw[len] = 0; /* Terminate it !! */
>
> - proffile = &nl[0];
> - }
> + wproffile = &nlw[0];
> }
> }
> #else
Watch out for the wproffile = &nlw[0] assignment! You are
storing in wproffile an address that will no longer be valid
once the scope that contains nlw ends, and you are using wprofille
afterwards. Surely some nasty surprises can show up here if
gcc decides to put something else in the same stack slot as nlw.
One possible solution is to move nlw into an outer scope.
You are assuming the ModuleName will be always ended in .exe or .dll,
which isn't true. You can have an executable with whatever extension you
like, not just .exe, and same for dlls. How about appending ".gmo" instead of
rewriting the extension?
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.
Like so in WinCE:
/Programs/Mydir/Myapp.exe -> /Programs/Mydir/gmon.out
That way every gmon tutorial out there will also apply to us. No special
casing.
> @@ -271,15 +269,21 @@
> #endif
>
> + FormatMessageW(FORMAT_MESSAGE_FROM_SYSTEM
> + | FORMAT_MESSAGE_ALLOCATE_BUFFER |
> FORMAT_MESSAGE_IGNORE_INSERTS,
> + NULL, GetLastError(),
> + 0, (LPTSTR)&buf, 0, NULL);
Leak. A FormatMessageW with FORMAT_MESSAGE_ALLOCATE_BUFFER should be
matched with a LocalFree.
> Index: mingw/profile/Makefile.in
> ===================================================================
> --- mingw/profile/Makefile.in (revision 835)
> +++ mingw/profile/Makefile.in (working copy)
> @@ -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.
> +#
> # mingw/profile/Makefile.in: This file is part of Mingw runtime.
> #
> # This makefile requires GNU make.
> @@ -28,41 +34,21 @@
> DISTFILES = Makefile.in configure configure.in gcrt0.c gmon.c gmon.h
> mcount.c \
> profil.c profil.h profile.h CYGWIN_LICENSE COPYING
>
> -CC = @CC@
> -# FIXME: Which is it, CC or CC_FOR_TARGET?
> -CC_FOR_TARGET = $(CC)
> -AS_FOR_TARGET = $(AS)
> -CFLAGS = @CFLAGS@
> -CXXFLAGS = @CXXFLAGS@
> +CC = ${target_alias}-gcc
> +CFLAGS = -DNO_UNDERSCORES # cegcc won't work
> +# CFLAGS = -D__COREDLL__ -DNO_UNDERSCORES # cegcc won't work
> +# CFLAGS = -DNO_UNDERSCORES
>
> -# compiling with Cygwin?
> -MNO_CYGWIN = @MNO_CYGWIN@
> -
> # Either crtdll (CRT_ID 1), msvcrt (CRT_ID 2) or coredll (CRT_ID 3).
> RUNTIME = @RUNTIME@
> CRT_ID = @CRT_ID@
>
> -# Needed for threading dll.
> -THREAD_DLL = @THREAD_DLL@
> -THREAD_DLL_VERSION = 10
> -THREAD_DLL_NAME = $(THREAD_DLL)$(THREAD_DLL_VERSION).dll
> -
> -W32API_INCLUDE = @W32API_INCLUDE@
> -INCLUDES = -I$(srcdir) -I$(srcdir)/../include \
> - -nostdinc \
> - -iwithprefixbefore include
> +INCLUDES = -I$(srcdir)
> ALL_CFLAGS = $(CFLAGS) $(W32API_INCLUDE) $(INCLUDES) $(MNO_CYGWIN)
> -ALL_CXXFLAGS = $(CXXFLAGS) $(W32API_INCLUDE) $(INCLUDES) -nostdinc++
> $(MNO_CYGWIN)
>
> -AS = @AS@
> -AR = @AR@
> -LD = @LD@
> +AR = $(target_alias)[EMAIL PROTECTED]@
> AR_FLAGS = rcv
> -RANLIB = @RANLIB@
> -DLLTOOL = @DLLTOOL@
> -DLLTOOLFLAGS =
> -DLLTOOL_FOR_TARGET = $(DLLTOOL)
> -DLLTOOL_FLAGS = --as $(AS_FOR_TARGET)
> +RANLIB = $(target_alias)[EMAIL PROTECTED]@
>
> LIBGMON_A = @LIBGMON_A@
> LIBGMON_OBJS = gmon.o mcount.o profil.o
> @@ -70,7 +56,6 @@
> ALL_CRT0S = gcrt0.o gcrt1.o gcrt2.o gcrt3.o
>
> LIBS = $(LIBGMON_A)
> -DLLS =
>
> all: $(LIBGMON_A)
>
> @@ -93,16 +78,16 @@
> $(CC) -D__COREDLL__ -c -o $@ $(CPPFLAGS) $(ALL_CFLAGS) $?
>
> #
> -# Dependancies
> +# Dependencies
> #
> -gmon.o: gmon.c gmon.h profile.h profil.h
> -mcount.o: mcount.c gmon.h profile.h
> -profil.o: profil.c profil.h
> +gmon.o: $(srcdir)/gmon.c $(srcdir)/gmon.h $(srcdir)/profile.h
> $(srcdir)/profil.h
> +mcount.o: $(srcdir)/mcount.c $(srcdir)/gmon.h $(srcdir)/profile.h
> +profil.o: $(srcdir)/profil.c $(srcdir)/profil.h
>
> -Makefile: Makefile.in config.status configure
> +Makefile: $(srcdir)/Makefile.in config.status $(srcdir)/configure
> $(SHELL) config.status
>
> -config.status: configure
> +config.status: $(srcdir)/configure
> $(SHELL) config.status --recheck
>
> info:
> @@ -112,19 +97,14 @@
> install-info: info
>
> install: all
> - $(mkinstalldirs) $(inst_libdir)
> + $(mkinstalldirs) $(tooldir)/lib
> for i in $(LIBS); do \
> - $(INSTALL_DATA) $$i $(inst_libdir)/$$i ; \
> + $(INSTALL_DATA) $$i $(tooldir)/lib/$$i ; \
> + $(RANLIB) $(tooldir)/lib/$$i ; \
> done
> for i in $(CRT0S); do \
> - $(INSTALL_DATA) $$i $(inst_libdir)/$$i ; \
> + $(INSTALL_DATA) $$i $(tooldir)/lib/$$i ; \
> done
> - for sub in . ; do \
> - $(mkinstalldirs) $(inst_includedir)/$$sub ; \
> - for i in $(srcdir)/$$sub/*.h ; do \
> - $(INSTALL_DATA) $$i $(inst_includedir)/$$sub/`basename $$i` ; \
> - done ; \
> - done
>
> clean:
> -rm -f $(LIBGMON_OBJS) $(ALL_CRT0S) $(LIBGMON_A)
> @@ -142,4 +122,3 @@
> @for i in $(DISTFILES); do\
> cp -p $(srcdir)/$$i $(distdir)/profile/$$i ; \
> done
> -
Yikes! Why are these changes needed? Ah, you used --target, instead of --host.
When building a lib, you should use --host, not --target. Can you try
it please?
Also, your setting of CFLAGS = -DNO_UNDERSCORES, is wrong. Consider what
happens when I do 'export CFLAGS="-g3 -O0"; configure.' I want my CFLAGS
to be respected, and -DNO_UNDERSCORES to be added always.
The right fix would be to add -DNO_UNDERSCORES to this switch in
configure.in:
case "$target_os" in
*mingw32crt*)
(...)
UNDERSCORES=
;;
*cygwin*)
(...)
UNDERSCORES=
;;
*cegcc* | *mingw32ce*)
CRT_ID=3
MNO_CYGWIN=
RUNTIME=coredll
CRT0S="gcrt3.o"
UNDERSCORES=-DNO_UNDERSCORES
;;
*)
(...)
;;
esac
And add:
AC_SUBST(UNDERSCORES)
And then in Makefile.in get it:
UNDERSCORES = @UNDERSCORES@
And use it:
ALL_CFLAGS = $(CFLAGS) $(UNDERSCORES) $(W32API_INCLUDE) $(INCLUDES)
$(MNO_CYGWIN)
ALL_CXXFLAGS = $(CXXFLAGS) $(UNDERSCORES) $(W32API_INCLUDE) $(INCLUDES)
-nostdinc++ $(MNO_CYGWIN)
I leave the final UNDERSCORES name to you :).
> Index: cegcc/cegccdll/Makefile
> ===================================================================
> --- cegcc/cegccdll/Makefile (revision 835)
> +++ cegcc/cegccdll/Makefile (working copy)
> @@ -96,6 +96,8 @@
> rm -f ${MY_DIR}/$(IMPLIB)
> rm -f ${MY_DIR}/$(IMPLIB).tmp
> $(AR) q ${MY_DIR}/$(IMPLIB) ${MY_DIR}/_tmp_static/*.o
> +# ls -l ${MY_DIR}/_tmp/*eprintf*
> + rm -f ${MY_DIR}/_tmp/_eprintf.o
> $(CC) -o $@ $(LDFLAGS) -shared -nostdlib \
> -Wl,--whole-archive ${MY_DIR}/_tmp/*.o ${LIB_PATH}/dllcrt1.o \
> -Wl,--no-whole-archive -lcegcc -lcoredll -liphlpapi \
After leaving the ligcc2.c patch out (comment about that below),
it would be better to add _eprintf.o to the list of
always static objects.
> Index: build-mingw32ce.sh
> ===================================================================
> --- build-mingw32ce.sh (revision 841)
> +++ build-mingw32ce.sh (working copy)
> @@ -249,6 +250,30 @@
> tar cf - images | (cd ${PREFIX}/share; tar xf -) || exit 1
> }
>
> +function build_profile()
> +{
> + echo ""
> + echo "BUILDING profiling libraries --------------------------"
> + echo ""
> + echo ""
> +
> + mkdir -p ${BUILD_DIR}/profile || exit 1
> + cd ${BUILD_DIR}/profile || exit 1
> +
> + PREV_CFLAGS=${CFLAGS}
> + export CFLAGS="-DNO_UNDERSCORES"
> +
And there you go. This is wrong. This means that if I want
to use some other CFLAGS I won't be able to.
> + ${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. )
> function build_all
> {
> build_binutils
> @@ -261,6 +286,7 @@
> build_gdb
> build_gdbstub
> build_docs
> + build_profile
> }
>
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.
> Index: gcc/gcc/gcov-io.c
> ===================================================================
> --- gcc/gcc/gcov-io.c (revision 835)
> +++ gcc/gcc/gcov-io.c (working copy)
> @@ -134,7 +134,9 @@
> return 0;
> #endif
>
> +#ifndef __MINGW32CE__
> setbuf (gcov_var.file, (char *)0);
> +#endif
>
There's setvbuf in coredll.dll, use that instead.
> return 1;
> }
> Index: gcc/gcc/libgcc2.c
> ===================================================================
> --- gcc/gcc/libgcc2.c (revision 835)
> +++ gcc/gcc/libgcc2.c (working copy)
> @@ -1789,6 +1789,8 @@
> #ifdef L_eprintf
> #ifndef inhibit_libc
>
> +#if ! (defined(__CEGCC__) || defined(__MINGW32CE__))
> +
> #undef NULL /* Avoid errors if stdio.h and our stddef.h mismatch. */
> #include <stdio.h>
>
> @@ -1803,6 +1805,7 @@
>
> #endif
> #endif
> +#endif
>
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. )
>
> #ifdef L_clear_cache
> Index: gcc/gcc/coverage.c
> ===================================================================
> --- gcc/gcc/coverage.c (revision 835)
> +++ gcc/gcc/coverage.c (working copy)
> @@ -776,6 +776,7 @@
> unsigned n_fns;
> const struct function_list *fn;
> tree string_type;
> + char *gcov_cross_prefix;
>
> /* Count the number of active counters. */
> for (n_ctr_types = 0, ix = 0; ix != GCOV_COUNTERS; ix++)
> @@ -811,12 +812,27 @@
> field = build_decl (FIELD_DECL, NULL_TREE, string_type);
> TREE_CHAIN (field) = fields;
> fields = field;
> - filename = getpwd ();
> - filename = (filename && da_file_name[0] != '/'
> +
> + /*
> + * Additional environment variable for cross-development.
> + */
> + if (gcov_cross_prefix = getenv ("GCOV_CROSS_PREFIX"))
> + {
> + fprintf(stderr, "Yow GCOV_CROSS_PREFIX(%s)\n", gcov_cross_prefix);
> + filename = concat (gcov_cross_prefix, "/", da_file_name, NULL);
> + filename_len = strlen (filename);
> + filename_string = build_string (filename_len + 1, filename);
> + }
> + else
> + {
> + filename = getpwd ();
> + filename = (filename && da_file_name[0] != '/'
> ? concat (filename, "/", da_file_name, NULL)
> : da_file_name);
> - filename_len = strlen (filename);
> - filename_string = build_string (filename_len + 1, filename);
> + filename_len = strlen (filename);
> + filename_string = build_string (filename_len + 1, filename);
> + }
> +
> if (filename != da_file_name)
> free (filename);
> TREE_TYPE (filename_string) = build_array_type
> Index: gcc/gcc/libgcov.c
> ===================================================================
> --- gcc/gcc/libgcov.c (revision 835)
> +++ gcc/gcc/libgcov.c (working copy)
> @@ -194,6 +194,12 @@
> }
> }
>
> +#ifdef __MINGW32CE__
> + /* No getenv support, so disable this. */
> + gcov_prefix = (char *)0;
> + gcov_prefix_strip = 0;
> + prefix_length = 0;
> +#else
> /* Get file name relocation prefix. Non-absolute values are ignored. */
> gcov_prefix = getenv("GCOV_PREFIX");
> if (gcov_prefix && IS_ABSOLUTE_PATH (gcov_prefix))
> @@ -216,6 +222,7 @@
> }
> else
> prefix_length = 0;
> +#endif
>
> /* Allocate and initialize the filename scratch space. */
> gi_filename = alloca (prefix_length + gcov_max_filename + 1);
> @@ -310,6 +317,15 @@
> continue;
> }
> #endif
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?
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?
> Index: gcc/gcc/tsystem.h
> ===================================================================
> --- gcc/gcc/tsystem.h (revision 835)
> +++ gcc/gcc/tsystem.h (working copy)
> @@ -92,12 +92,14 @@
> /* All systems have this header. */
> #include <sys/types.h>
>
> -/* All systems have this header. */
> +#ifndef UNDER_CE
> +/* All systems except Windows CE have this header. */
> #include <errno.h>
>
> #ifndef errno
> extern int errno;
> #endif
> +#endif
>
s/UNDER_CE/__MINGW32CE__/ . Cegcc does have errno.h, and
a real 'int errno', because it uses newlib for the libc functions.
Also, I don't like having a target define where there is none already.
This is one of those patches that will have to be fixed
before submission to upstream gcc. It will surelly be rejected.
I have no idea if the correct fix would be to wrap the
errno.h inclusion in HAVE_ERRNO_H, or if it would be better
to just scrap the include for all targets, and fix the other places
where is was needed. Let's leave the __MINGW32CE__ for now, then...
Cheers,
Pedro Alves
-------------------------------------------------------------------------
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
