March 2016 - Wine-Devel - list.winehq.org

Re: libport: Mark internal functions and data tables as hidden.

by Francois Gouget

commit 34174e2ca8cb61150d3d880400d8231a39d38307 Author: Alexandre Julliard <julliard(a)winehq.org> Date: Wed Feb 24 13:36:41 2016 +0900 libport: Mark internal functions and data tables as hidden. This patch breaks the compilation on Solaris 11.11: ld: fatal: relocation error: R_386_GOTOFF: file ../../libs/port/libwine_port.a(fold.o): symbol wine_digitmap: a GOT relative relocation must reference a local symbol ld: fatal: relocation error: R_386_GOTOFF: file ../../libs/port/libwine_port.a(fold.o): symbol wine_compatmap: a GOT relative relocation must reference a local symbol Removing the DECLSPEC_HIDDEN on wine_digitmap(), wine_compatmap() but also wine_compose() and wine_decompose() fixes this. As far as I can tell the linker considers that hidden symbols are not visible outside the static library. -- Francois Gouget <fgouget(a)free.fr> http://fgouget.free.fr/ A black hole is just God dividing by zero.

March 1, 2016

Re: [PATCH 2/4] gdi32: Return NTM_DSIG when table is present

by Huw Davies

On Tue, Mar 01, 2016 at 07:19:02AM +0300, Nikolay Sivov wrote: > Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> > --- > dlls/gdi32/freetype.c | 13 ++++++++++--- > dlls/gdi32/tests/font.c | 3 +++ > 2 files changed, 13 insertions(+), 3 deletions(-) > > diff --git a/dlls/gdi32/freetype.c b/dlls/gdi32/freetype.c > index 01cb913..57cf695 100644 > --- a/dlls/gdi32/freetype.c > +++ b/dlls/gdi32/freetype.c > @@ -1942,7 +1942,6 @@ static inline FT_Fixed get_font_version( FT_Face ft_face ) > static inline DWORD get_ntm_flags( FT_Face ft_face ) > { > DWORD flags = 0; > - FT_ULong table_size = 0; > FT_WinFNT_HeaderRec winfnt_header; > > if (ft_face->style_flags & FT_STYLE_FLAG_ITALIC) flags |= NTM_ITALIC; > @@ -1956,8 +1955,16 @@ static inline DWORD get_ntm_flags( FT_Face ft_face ) > > if (flags == 0) flags = NTM_REGULAR; > > - if (!pFT_Load_Sfnt_Table( ft_face, FT_MAKE_TAG( 'C','F','F',' ' ), 0, NULL, &table_size )) > - flags |= NTM_PS_OPENTYPE; > + if (FT_IS_SFNT( ft_face )) { > + FT_ULong table_size = 0; > + > + if (!pFT_Load_Sfnt_Table( ft_face, FT_MAKE_TAG( 'C','F','F',' ' ), 0, NULL, &table_size )) > + flags |= NTM_PS_OPENTYPE; > + > + table_size = 0; > + if (!pFT_Load_Sfnt_Table( ft_face, FT_MAKE_TAG( 'D','S','I','G' ), 0, NULL, &table_size )) > + flags |= NTM_DSIG; > + } > > return flags; > } > diff --git a/dlls/gdi32/tests/font.c b/dlls/gdi32/tests/font.c > index d39e701..1059d29 100644 > --- a/dlls/gdi32/tests/font.c > +++ b/dlls/gdi32/tests/font.c > @@ -78,6 +78,7 @@ static WORD system_lang_id; > #define MS_CMAP_TAG MS_MAKE_TAG('c','m','a','p') > #define MS_NAME_TAG MS_MAKE_TAG('n','a','m','e') > #define MS_CFF_TAG MS_MAKE_TAG('C','F','F',' ') > +#define MS_DSIG_TAG MS_MAKE_TAG('D','S','I','G') > > static void init(void) > { > @@ -3968,6 +3969,8 @@ static void test_text_metrics(const LOGFONTA *lf, const NEWTEXTMETRICA *ntm) > has_cff = GetFontData(hdc, MS_CFF_TAG, 0, NULL, 0) != GDI_ERROR; > ok(((ntm->ntmFlags & NTM_PS_OPENTYPE) == NTM_PS_OPENTYPE) == has_cff, "CFF test, got flags 0x%08x, CFF table %d, %s\n", > ntm->ntmFlags, has_cff, font_name); > + if (GetFontData(hdc, MS_DSIG_TAG, 0, NULL, 0) != GDI_ERROR) > + ok(ntm->ntmFlags & NTM_DSIG, "DSIG test, got flags 0x%08x, %s\n", ntm->ntmFlags, font_name); > > test_negative_width(hdc, lf); I get a test failure on OS/X. I suspect it might be something to do with the Helvetica -> Arial substitution. font.c:3974: Test failed: DSIG test, got flags 0x00040040, Helvetica Also, since you'll have to resend it, could you avoid adding more K&R style braces. Huw.

March 1, 2016

Re: [v5 PATCH] comctl32/tooltip: Protect TTM_ADDTOOLW from invalid text pointers

by Nikolay Sivov

On 01.03.2016 9:46, Alistair Leslie-Hughes wrote: > diff --git a/dlls/comctl32/tooltips.c b/dlls/comctl32/tooltips.c > index 8bf6919..43c02ee 100644 > --- a/dlls/comctl32/tooltips.c > +++ b/dlls/comctl32/tooltips.c > @@ -1038,7 +1038,9 @@ TOOLTIPS_AddToolT (TOOLTIPS_INFO *infoPtr, const TTTOOLINFOW *ti, BOOL isW) > infoPtr->hwndSelf, ti->hwnd, ti->uId, > (ti->uFlags & TTF_IDISHWND) ? " TTF_IDISHWND" : ""); > > - if (ti->cbSize >= TTTOOLINFOW_V2_SIZE && !ti->lpszText && isW) > + if (ti->cbSize >= TTTOOLINFOW_V2_SIZE && isW && (!ti->lpszText || > + (ti->lpszText != LPSTR_TEXTCALLBACKW && > + IsBadStringPtrW(ti->lpszText, sizeof(WCHAR))))) > return FALSE; > > if (infoPtr->uNumTools == 0) { > IsBadStringPtrW() won't do what you expect, if you expect it to validate whole string up to NULL char. Also !ti->lpszText case is covered by IsBadStringPtrW(), you don't need to test for it separately.

March 1, 2016

Re: [PATCH] user32/text: Stop crash when Tab Length is zero

by Alexandre Julliard

Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> writes: > @@ -948,7 +948,7 @@ INT WINAPI DrawTextExW( HDC hdc, LPWSTR str, INT i_count, > > if (flags & DT_EXPANDTABS) > { > - int tabstop = ((flags & DT_TABSTOP) && dtp) ? dtp->iTabLength : 8; > + int tabstop = ((flags & DT_TABSTOP) && dtp && dtp->iTabLength) ? dtp->iTabLength : 8; You should test the resulting text size to show that this is the correct behavior. -- Alexandre Julliard julliard(a)winehq.org

March 1, 2016

Re: [PATCH 2/4] gdi32: Return NTM_DSIG when table is present

by Nikolay Sivov

On 01.03.2016 7:37, Marvin wrote: > Hi, > > While running your changed tests on Windows, I think I found new failures. > Being a bot and all I'm not very good at pattern recognition, so I might be > wrong, but could you please double-check? > Full results can be found at > https://testbot.winehq.org/JobDetails.pl?Key=20990 > > Your paranoid android. > > > === wvistau64 (32 bit font) === > An error occurred while waiting for the test to complete: network read timed out (Connect:AgentVersion.h:0/9) > > This and next one for patch 3/4 is obviously some generic bot failure, manual run worked.

March 1, 2016

Re: [v5 PATCH] comctl32/tooltip: Protect TTM_ADDTOOLW from invalid text pointers

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=20992 Your paranoid android. === wxppro (32 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed! === w2003std (32 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed! === wvistau64 (32 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed! === w2008s64 (32 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed! === w7u (32 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed! === w7pro64 (32 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed! === w8 (32 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed! === w864 (32 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed! === w1064 (32 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed! === wvistau64 (64 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed! === w2008s64 (64 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed! === w7pro64 (64 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed! === w864 (64 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed! === w1064 (64 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed!

March 1, 2016

Re: [PATCH 3/4] gdi32: Set NTM_TT_OPENTYPE flag for fonts with TrueType outlines

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=20991 Your paranoid android. === wvistau64 (32 bit font) === An error occurred while waiting for the test to complete: network read timed out (Connect:AgentVersion.h:0/9)

March 1, 2016

Re: [PATCH 2/4] gdi32: Return NTM_DSIG when table is present

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=20990 Your paranoid android. === wvistau64 (32 bit font) === An error occurred while waiting for the test to complete: network read timed out (Connect:AgentVersion.h:0/9)

March 1, 2016

Re: [PATCH 2/3] user32: Added more dialog owner tests.

by Dmitry Timoshkov

Alexandre Julliard <julliard(a)winehq.org> wrote: > > Jacek Caban <jacek(a)codeweavers.com> wrote: > >> I'm not sure I agree. > > > > Please don't get me wrong, but when sending tests after the fixes the result > > can't be easily verified, and therefore this reduces the trust and value of > > patches. > > There are many cases where adding todos and then removing them creates > more work, so it's perfectly acceptable to send a test to apply after > the fix to demonstrate that it is correct. We do that all the time. In this case just adding and then removing TRUE in ok_sequence() call and adding and then removing appropriate todo_wine statements shouldn't create too much additional work, after all this work should be done anyway in order to verify that the patch actually works. > If you don't trust that the test is testing the right thing, it's easy > enough to verify by reverting the fix and confirming that the test > fails. I do that frequently myself. As I have said, the patches that remove todo_wine statements are easier recognizable as the fixes. -- Dmitry.

March 1, 2016

Re: [PATCH 2/3] user32: Added more dialog owner tests.

by Dmitry Timoshkov

Hi Jacek, Jacek Caban <jacek(a)codeweavers.com> wrote: > >>>>> Please don't create fake embedded blocks. > >>>> I need this declaration here, otherwise I wouldn't be able to use > >>>> handles for testing lparam value. > >>> wparam/lparam could be tested directly in the window procedure. > >> It would require some global variables and new window procedure. It's > >> cleaner to do it using existing tools IMO. > > Writing the tests for user32 requires a bit of efforts, yes. > > I'd prefer to respond to comments that intend to improve the patch > instead of comments that intend to increase efforts. I already described the ways how to improve the patch. Improving a patch and writing good tests usually increases the efforts, not avoids them. > > But since > > you are new to this area I think it's justified to ask for more tests > > to show that you well understand the code and can proof that the patch > > is 100% correct. > > Given quality of your comments so far (and I mean technical ones, not > just this one) I think it's justified to ask you to go back and try to > understand my patch before questioning my abilities. I'm not qustioning your abilities, just pointing out to an obvious fact. Since I'm the person who spent a lot of time working on user32 stuff and who has written many of the exisitng user32 tests I think that asking for a more convincing test shouldn't lead to so a much exagerrated reaction. -- Dmitry.

March 1, 2016