- Wine-Devel - list.winehq.org

[PATCH] winemaker: fix catch() and throw() in winelib DLLs
by Martin Wilck Sept. 10, 2002

Sept. 10, 2002

Patch: winemaker-wrapper.diff Martin Wilck <Martin.Wilck(a)fujitsu-siemens.com> Patch against: CVS 2002-09-10 Modified files: - tools: winemaker * A wrapper binary should not be linked to any library passed with "-l". [ I am forwarding this to wine-devel because the problem may be deeper than I think, see below ]. Rationale: This fixes a delicate problem with catch() and throw() in a DLL used by a winelib application that has a wrapper app. I have written this small, hopefully self-explaining, shell script that demonstrates the problem. (Note that my previous winemaker patches are necessary otherwise there'll be other errors before this one comes into play). #! /bin/sh # throwtest.sh - test winemaker with catch()/throw() in a DLL # Change this to the location of your wine installation (DLL will also be installed there) WINEDIR=$HOME/exp # Extra winemaker options to build EXE: CORE DUMP with --wrap, fine without EXEOPTS="--wrap" # Extra winemaker options to build DLL DLLOPTS="" errexit () { echo "** Error $1 !! **" >&2 exit 1 } export PATH=$WINEDIR/bin:$PATH export LD_LIBRARY_PATH=$WINEDIR/lib:$LD_LIBRARY_PATH echo "Building library ..." >&2 mkdir -p trylib cd trylib rm -f * # BEWARE !!! cat > trylib.cpp <<EOF int do_try (void) { int ret = 0; try { throw(1); } catch (int e) { ret = e; } return ret; } EOF winemaker --dll $DLLOPTS . > winemaker.out 2>&1 || errexit "running winemaker for library" ## remove the "init DllMain" declaration winemaker generates echo "" >libtrylib.spec ./configure --with-wine=$WINEDIR --prefix=$WINEDIR > conf.log 2>&1 || errexit "during library configure" make >make.log 2>&1 || errexit "building library" make install >>make.log 2>&1 || errexit "installing library" cd .. echo "Building program ..." >&2 mkdir -p tryexe cd tryexe rm -f * # BEWARE !!! cat > tryexe.cpp <<EOF #include <stdio.h> extern int do_try (void); void main (void) { printf ("Result: %d\n", do_try()); } EOF winemaker --cuiexe $EXEOPTS -ltrylib . > winemaker.out 2>&1 || errexit "running winemaker for program" ./configure --with-wine=$WINEDIR --prefix=$WINEDIR > conf.log 2>&1 || errexit "during program configure" make >make.log 2>&1 || errexit "building program" echo "Running program ..." >&2 ./tryexe >run.out 2>&1 || errexit "running program" echo "** Success !! **" >&2 # EOF throwtest.sh On my system, this yields the following output: Building library ... Building program ... Running program ... throwtest.sh: line 65: 14430 Aborted (core dumped) ./tryexe >run.out 2>&1 ** Error running program !! ** The reason is that "-ltrylib" becomes part of the DLL_LINK make variable and the wrapper app is thus linked with "libtrylink.so". If my analysis is correct, this leads to a situation where the gcc-generated exception code for throw() jumps into the wrapper app rather than into the wrapped app - i.e. to nowhere. IMO it makes no sense to include any additional libraries in the link list for the wrapper app (=> patch). If this isn't done, the test runs fine. This may point to a conceptual problem with the wrapper app approach, but at least it seems to run fine with the patch below. Index: tools/winemaker =================================================================== RCS file: /home/wine/wine/tools/winemaker,v retrieving revision 1.47 diff -u -r1.47 winemaker --- tools/winemaker 10 Sep 2002 00:42:43 -0000 1.47 +++ tools/winemaker 10 Sep 2002 18:21:57 -0000 @@ -1993,7 +1993,11 @@ } else { print FILEO "\t\$(LDSHARED)"; } - print FILEO " \$(LDDLLFLAGS) -o \$\@ \$(${canon}_OBJS) \$(${canon}_SPEC_SRCS:.spec=.spec.o) \$(${canon}_LIBRARY_PATH) \$(${canon}_LIBRARIES:%=-l%) \$(DLL_LINK) \$(LIBS)\n"; + if (@$target[$T_FLAGS] & $TF_WRAPPER) { + print FILEO " \$(LDDLLFLAGS) -o \$\@ \$(${canon}_OBJS) \$(${canon}_SPEC_SRCS:.spec=.spec.o) \$(${canon}_LIBRARY_PATH) \$(${canon}_LIBRARIES:%=-l%) \$(WRAPPER_DLL_LINK) \$(LIBS)\n"; + } else { + print FILEO " \$(LDDLLFLAGS) -o \$\@ \$(${canon}_OBJS) \$(${canon}_SPEC_SRCS:.spec=.spec.o) \$(${canon}_LIBRARY_PATH) \$(${canon}_LIBRARIES:%=-l%) \$(DLL_LINK) \$(LIBS)\n"; + } if (@$target[$T_TYPE] ne $TT_DLL) { print FILEO "\ttest -f @$target[$T_NAME] || \$(INSTALL_SCRIPT) wineapploader @$target[$T_NAME]\n"; } @@ -3043,6 +3047,7 @@ ALLCFLAGS = $(CFLAGS) $(CEXTRA) $(OPTIONS) $(ALLFLAGS) ALLCXXFLAGS=$(CXXFLAGS) $(CXXEXTRA) $(OPTIONS) $(ALLFLAGS) ALLWRCFLAGS=$(WRCFLAGS) $(WRCEXTRA) $(OPTIONS) $(ALLFLAGS) +WRAPPER_DLL_LINK = $(LIBRARY_PATH) $(WINE_LIBRARY_PATH) -lwine -lwine_unicode -lwine_uuid DLL_LINK = $(LIBRARY_PATH) $(LIBRARIES:%=-l%) $(WINE_LIBRARY_PATH) -lwine -lwine_unicode -lwine_uuid LDCOMBINE = ld -r LDSHARED = @LDSHARED@ -- Martin Wilck Phone: +49 5251 8 15113 Fujitsu Siemens Computers Fax: +49 5251 8 20409 Heinz-Nixdorf-Ring 1 mailto:Martin.Wilck(a)Fujitsu-Siemens.com D-33106 Paderborn http://www.fujitsu-siemens.com/primergy

1 0

RE: mssip.h trouble
by Patrik Stridvall Sept. 10, 2002

Sept. 10, 2002

> Patrik Stridvall wrote: > > > On MSVC it does matter in the following example whether n is 4 or 8. > > Or 16 for that matter eventhough that gives the same result as 8. > > > > #ifdef _MSC_VER > > typedef __int64 longlong_t; > > #else > > typedef long long longlong_t; > > #endif > > > > #pragma pack(n) > > typedef struct { > > char x; > > longlong_t y; /* offset is 4 if n = 4, but is 8 if n = > 8 or n = 16*/ > > } longlong_n; > > #pragma pack() > > I would assume that MSVC and GCC use different structure > layouts even without any #pragma pack(), because the > default aligment of 8-byte data types is different. > > As I mentioned in my other mail, GCC should use the same > layout as MSVC if you compile with -malign-double. As I mention in my other mail you are absolutely correct. :-) So it seems that if we add a -malign-double to CFLAGS we can remove the warning in pshpack8.h since now it will hopefully work correctly.

2 1

RE: mssip.h trouble
by Patrik Stridvall Sept. 10, 2002

Sept. 10, 2002

> Ah, I guess I see the problem: it doesn't really have > anything to do with #pragma pack(), but with the default > alignment of the 'long long' data type itself. > > GCC uses a default alignment of 4 bytes for 'long long', > so, if MSVC uses a default alignment of 8, that would be > the problem. You seem to be correct. With this code: #ifdef _MSC_VER typedef __int64 longlong_t; #else typedef __attribute__((aligned (8))) long long longlong_t; #endif GNU C now works the same as with MSVC. > However, I think you can get GCC on i386 to use a default > alignment of 8 for 8-byte sized types, by specifying the > compiler option -malign-double. Yes, that works with GNU C as well. Even without my hack above. > (Then you'll probably > have to use -mpreferred-stack-boundary=3 instead of > -mpreferred-stack-boundary=2 as well.) It doesn't seem to do any difference in my simple example either with -mpreferred-stack-boundary=2 or -mpreferred-stack-boundary=3. The only thing that matters is whether -malign-double is set.

1 0

RE: mssip.h trouble
by Patrik Stridvall Sept. 10, 2002

Sept. 10, 2002

> Patrik Stridvall wrote: > > > - No GNU C compiler not even 3.1 support #pragma pack(8) nor > > does it support #pragma pack(16). It doesn't even give a warning. > > Well, GCC does support #pragma pack(8) and #pragma pack(16). > They just don't do anything on i386, because they don't need to. > > What #pragma pack(N) means is: if any structure member would > require a default alignment > N according to the normal rules, > reduce that alignment to N. > > As on i386 no structure member ever requires a default alignment > of > 8, #pragma pack(8) never needs to do anything. > > You appear to think that #pragma pack(N) should sometimes > *increase* some alignment requirement -- but it never does. That sound reasonable. However Microsoft doesn't seem to be reasonable. :-) > Under what circumstances do you think it should? Actually I don't think anything. I have tested what different compilers does. :-) As I explain in my latest mail (that perhaps you didn't read since it was only 5 minutes between the mails :-). On MSVC it does matter in the following example whether n is 4 or 8. Or 16 for that matter eventhough that gives the same result as 8. #ifdef _MSC_VER typedef __int64 longlong_t; #else typedef long long longlong_t; #endif #pragma pack(n) typedef struct { char x; longlong_t y; /* offset is 4 if n = 4, but is 8 if n = 8 or n = 16*/ } longlong_n; #pragma pack()

2 1

RE: mssip.h trouble
by Patrik Stridvall Sept. 10, 2002

Sept. 10, 2002

> Patrik Stridvall <ps(a)leissner.se> writes: > > > I'm totally at loss as to what to do. Not that it currently > matters since > > the data structures isn't currently used but someday > somebody will... > > Well, we need to add explicit padding in the structures to respect the > alignment that MSVC uses. That should only be required at the end of > the structures I think, so it's not too bad. Unfortunately its a little more complicated than that, it effects the internal offset of fields as well in some case. In the normal case below it doesn't matter whether n is 4 or 8. #pragma pack(n) typedef struct { char x; long y; /* offset is 4 both for n = 4 and n = 8 */ } long_n; #pragma pack() However is the case below is does matter whether n is 4 or 8: #ifdef _MSC_VER typedef __int64 longlong_t; #else typedef long long longlong_t; #endif #pragma pack(n) typedef struct { char x; longlong_t y; /* offset is 4 if n = 4, but is 8 if n = 8 */ } longlong_n; #pragma pack() In short, if the size of a field is larger than 4 it matters internally whether alignment is 4 or 8. Now in the case of mssip.h, I can't see any fields that have sizes larger than 4 so we could easily handle that special case as you suggested. However in general we can not...

2 1

RE: mssip.h trouble
by Patrik Stridvall Sept. 10, 2002

Sept. 10, 2002

> On Tue, 10 Sep 2002, Patrik Stridvall wrote: > > > > While compiling the latest tree, I get this: > > > > > > [dimi(a)dimi wine]$ make -s > > > In file included from ../../include/mssip.h:49, > > > from main.c:22: > > > ../../include/pshpack8.h:47:6: warning: #warning "8 as > > > alignment is not supported" > [...] > > It seems that gcc or at least a previous versions of GCC > > didn't support it since somebody (Alexandre?) added a > > warning in pshpack8.h. > > It may have been me. I think at the time it was only meant for gcc, > which was the only things you could compile Wine with anyway. > The specific version of gcc is 2.95, and but I'm really not sure the > situation has changed. It hasn't. GCC 3.1 has the same problem. > > Futhermore Micrsoft C/C++ (or at least later version of it) > > contrary to the claim of pshpack8.h support it so it > > wrong at the point at least AFAIK. > > Yes, Visual C++ supports alignment 8 which is the problem (so that > defined(_MSC_VER) should have been moved elsewhere, just to avoid the > warning in fact). > > How we can handle such alignment issues in Wine whn compiled with gcc > I'm not sure. Neither am I. I have tested the alignment with various compiler and got the follow result: - No GNU C compiler not even 3.1 support #pragma pack(8) nor does it support #pragma pack(16). It doesn't even give a warning. - MSVC 6.0 support #pragma pack(8) but not #pragma pack(16) - The Solaris compiler (old version) doesn't even support #pragma pack(4), but then 4 is the default so it doesn't really matter that much (except for a warning). It support #pragma pack(1) and #pragma pack(2) though. So the conclusion is that only MSVC support #pragma pack(8), nothing else. I'm totally at loss as to what to do. Not that it currently matters since the data structures isn't currently used but someday somebody will... PS. I can't understand why GNU C doesn't support it, it can't be that difficult to implement can it? Nether the less that fact that it doesn't warn about it is even worse. That certainly can't be difficult to implement.

5 4

RE: mssip.h trouble
by Patrik Stridvall Sept. 10, 2002

Sept. 10, 2002

> While compiling the latest tree, I get this: > > [dimi(a)dimi wine]$ make -s > In file included from ../../include/mssip.h:49, > from main.c:22: > ../../include/pshpack8.h:47:6: warning: #warning "8 as > alignment is not supported" > In file included from ../../include/mssip.h:80, > from main.c:22: > ../../include/pshpack8.h:47:6: warning: #warning "8 as > alignment is not supported" > In file included from ../../include/mssip.h:88, > from main.c:22: > ./../include/pshpack8.h:47:6: warning: #warning "8 as > alignment is not supported" > In file included from ../../include/mssip.h:97, > from main.c:22: > ../../include/pshpack8.h:47:6: warning: #warning "8 as > alignment is not supported" > In file included from ../../include/mssip.h:109, > from main.c:22: > ../../include/pshpack8.h:47:6: warning: #warning "8 as > alignment is not supported" > In file included from ../../include/mssip.h:123, > from main.c:22: > ../../include/pshpack8.h:47:6: warning: #warning "8 as > alignment is not supported" > In file included from ../../include/mssip.h:140, > from main.c:22: > ../../include/pshpack8.h:47:6: warning: #warning "8 as > alignment is not supported" Ah, yes. I forgot to investigate that issue further. Sorry. First of all I want so say that the warning in itself is harmless. However anything using data structures from the header might not work correctly, but then currently only stub functions use it so it doesn't really matter. The problem is that mssip.h needs 8 as alignement for its data structures. It seems that gcc or at least a previous versions of GCC didn't support it since somebody (Alexandre?) added a warning in pshpack8.h. Futhermore Micrsoft C/C++ (or at least later version of it) contrary to the claim of pshpack8.h support it so it wrong at the point at least AFAIK. The big question is how should the following lines look like: # if defined(__GNUC__) || defined(__SUNPRO_C) || defined(__SUNPRO_CC) || defined(_MSC_VER) # pragma pack(8) # warning "8 as alignment is not supported" # elif !defined(RC_INVOKED) # error "Adjusting the alignment is not supported with this compiler" # endif Or perhaps phrased in another way: What compiler and specifically what versions or the compiler supports and handles #pragma pack(8) correctly? Note that just because it compiles without warning (or error) doesn't mean that it supports it correctly.

2 1

patches policy
by Dimitrie O. Paun Sept. 10, 2002

Sept. 10, 2002

Hi all, There is something concerning submitting patches that bothers me to no end: inlining vs. attaching them. I don't know about others, but for me 99/1 rule applies: I at least skim over 99% of the patches inlines in the message, whereas I bother to read at most 1% of the ones attached/tar.gzed/ziped. Maybe I'm an extreme case, but there's got to be more to it than my personal quirks. If a patch is sent to wine-patches, it's sent there for peer review. If you don't want the review, send it to Alexandre directly, even though I suggest this is avoided as much as possible. However, it you do send it to wine-patches, please, *please* inline it! What about a nice patch submission policy: -- unified diff only (required) -- have a decent subject (recommended) -- a long description (optional, if the change warrants it) -- a meaningful ChangeLog entry (required) -- new files, if any, included in patch, diffed against /dev/null (required) -- patch inlined at the end of the message (required) -- one changeset per message Most of these things are already followed by most people, with the exception of the inlining bit. Alexandre, what about you reject patches that aren't in this format with a pointer to these rules? All this is a matter of habit, and I think we'll all benefit if these rules are followed. -- Dimi.

9 12

mssip.h trouble
by Dimitrie O. Paun Sept. 10, 2002

Sept. 10, 2002

While compiling the latest tree, I get this: [dimi(a)dimi wine]$ make -s In file included from ../../include/mssip.h:49, from main.c:22: ../../include/pshpack8.h:47:6: warning: #warning "8 as alignment is not supported" In file included from ../../include/mssip.h:80, from main.c:22: ../../include/pshpack8.h:47:6: warning: #warning "8 as alignment is not supported" In file included from ../../include/mssip.h:88, from main.c:22: ../../include/pshpack8.h:47:6: warning: #warning "8 as alignment is not supported" In file included from ../../include/mssip.h:97, from main.c:22: ../../include/pshpack8.h:47:6: warning: #warning "8 as alignment is not supported" In file included from ../../include/mssip.h:109, from main.c:22: ../../include/pshpack8.h:47:6: warning: #warning "8 as alignment is not supported" In file included from ../../include/mssip.h:123, from main.c:22: ../../include/pshpack8.h:47:6: warning: #warning "8 as alignment is not supported" In file included from ../../include/mssip.h:140, from main.c:22: ../../include/pshpack8.h:47:6: warning: #warning "8 as alignment is not supported" nm: devenum.dll.tmp.o: no symbols nm: dplay.dll.tmp.o: no symbols -- Dimi.

1 0

Re: winedbg: enhancing symbol lookup
by Alexandre Julliard Sept. 10, 2002

Sept. 10, 2002

Eric Pouech <eric.pouech(a)wanadoo.fr> writes: > --- debugger/break.c 30 Jul 2002 00:06:34 -0000 1.35 > +++ debugger/break.c 2 Sep 2002 19:39:59 -0000 > @@ -421,10 +421,15 @@ > DBG_VALUE value; > int i; > > - if (DEBUG_GetSymbolValue(name, lineno, &value, TRUE)) > + switch (DEBUG_GetSymbolValue(name, lineno, &value, TRUE)) > { > + case TRUE: > DEBUG_AddBreakpoint(&value, NULL, TRUE); > return; > + case FALSE: > + break; > + case ABORT: /* user aborted symbol lookup */ > + return; > } Please don't invent a third boolean state. A boolean is either TRUE or FALSE; if you need more than that you should define a new enum type with meaningful names. Reusing TRUE and FALSE will cause major confusion. -- Alexandre Julliard julliard(a)winehq.com

1 0