Re: [Libreoffice] [PATCH] fdo#34908
Hi Lubos
On 25/03/11 18:52, Lubos Lunak wrote:
printf( "%p %p %s %p %p %s %p %p %s\n", ptr, dynamic_cast< void*>( ptr ),
typeid( *ptr ).name(), pFieldmark, dynamic_cast< void*>( pFieldmark ),
typeid( *pFieldmark ).name(), pCheckboxFm, dynamic_cast< void*>(
pCheckboxFm ), typeid( *pCheckboxFm ).name());
(where 'ptr' is what you get from the pMarksAccess->makeNoTextFieldBookmark()
call before casting to anything)
Okay, managed to put in some debug
here the code snippet(s) were the debug was done ( same for
distro/no-distro except for some text to distinguish one from the other )
ww8par3.cxx: ( SwWW8ImplReader::Read_F_FormCheckBox )
[...]
if (aBookmarkName.Len()>0)
{
IDocumentMarkAccess* pMarksAccess = rDoc.getIDocumentMarkAccess( );
//IFieldmark* pFieldmark = dynamic_cast(
pMarksAccess->makeNoTextFieldBookmark(
IFieldmark* ptr = ( pMarksAccess->makeNoTextFieldBookmark(
*pPaM, aBookmarkName,
rtl::OUString::createFromAscii( ODF_FORMCHECKBOX ) ) );
IFieldmark* pFieldmark = dynamic_cast( ptr );
OSL_ENSURE(pFieldmark!=NULL, "hmmm; why was the bookmark not
created?");
if (pFieldmark!=NULL) {
IFieldmark::parameter_map_t* const pParameters =
pFieldmark->GetParameters();
ICheckboxFieldmark* pCheckboxFm =
reinterpret_cast(pFieldmark);
OSL_TRACE( "distro ( Read_F_FormCheckBox ) %p %p %s %p %p
%s %p %p %s reinterpret %p %s\n",
ptr, dynamic_cast< void* >( ptr ), typeid( *ptr ).name(),
pFieldmark, dynamic_cast< void* >( pFieldmark ),
typeid( *pFieldmark ).name(),
pCheckboxFm, dynamic_cast< void* >( pCheckboxFm ),
typeid( *pCheckboxFm ).name(),
reinterpret_cast( ptr ), typeid(
(*reinterpret_cast( ptr ))).name() );
OSL_TRACE("dynamic_cast( ptr ) %p",
dynamic_cast( ptr ));
[...]
docbm.cxx ( MarkManager::makeNoTextFieldBookmark )
[...]
sw::mark::IMark* pMark = makeMark( rPaM, rName,
IDocumentMarkAccess::CHECKBOX_FIELDMARK );
sw::mark::IFieldmark* pFieldMark =
dynamic_cast( pMark );
sw::mark::ICheckboxFieldmark* pCBmark =
dynamic_cast( pMark );
OSL_TRACE( "distro (makeNoTextFieldBookmark ) %p %p %s %p %p
%s \n", pMark, dynamic_cast< void* >( pMark ), typeid( *pMark ).name(),
pCBmark, dynamic_cast< void* >( pCBmark ), typeid( *pCBmark ).name( )
[...]
and the results..
disto 32-bit
distro (makeNoTextFieldBookmark ) 0x8d013a4 0x8d01370
N2sw4mark17CheckboxFieldmarkE 0x8d013a4 0x8d01370
N2sw4mark17CheckboxFieldmarkE
(interesting in the library the implementation is in the dynamic_cast
works )
distro 0x8d013a4 0x8d01370 N2sw4mark17CheckboxFieldmarkE 0x8d013a4
0x8d01370 N2sw4mark17CheckboxFieldmarkE 0x8d013a4 0x8d01370
N2sw4mark17CheckboxFieldmarkE reinterpret 0x8d013a4
N2sw4mark17CheckboxFieldmarkE
dynamic_cast( ptr ) (nil)
( and here it doesn't so I guess there is some symbol confusion.. but I
don't see it )
output from nm:
nm unxlngi6.pro/lib/libswli.so | c++filt | grep ICheckboxField
00343c90 t sw::mark::ICheckboxFieldmark::ICheckboxFieldmark()
00344bdc t sw::mark::ICheckboxFieldmark::~ICheckboxFieldmark()
00344d98 t sw::mark::ICheckboxFieldmark::~ICheckboxFieldmark()
00344f36 t sw::mark::ICheckboxFieldmark::~ICheckboxFieldmark()
00ebc9c0 d construction vtable for
sw::mark::ICheckboxFieldmark-in-sw::mark::CheckboxFieldmark
00ebe3a0 V construction vtable for
sw::mark::IFieldmark-in-sw::mark::ICheckboxFieldmark
00ebe460 V construction vtable for
sw::mark::IMark-in-sw::mark::ICheckboxFieldmark
00ebdea0 V typeinfo for sw::mark::ICheckboxFieldmark
00d4ef30 V typeinfo name for sw::mark::ICheckboxFieldmark
00ebe360 V VTT for sw::mark::ICheckboxFieldmark
00ebe280 V vtable for sw::mark::ICheckboxFieldmark
00344bbe t virtual thunk to
sw::mark::ICheckboxFieldmark::~ICheckboxFieldmark()
00344d7a t virtual thunk to
sw::mark::ICheckboxFieldmark::~ICheckboxFieldmark()
00344bcd t virtual thunk to
sw::mark::ICheckboxFieldmark::~ICheckboxFieldmark()
00344d89 t virtual thunk to
sw::mark::ICheckboxFieldmark::~ICheckboxFieldmark()
nm unxlngi6.pro/lib/libmswordli.so | c++filt | grep ICheckboxField
0023b9b8 V typeinfo for sw::mark::ICheckboxFieldmark
001f303c V typeinfo name for sw::mark::ICheckboxFieldmark
non - disto 32-bit
==
no-distro (makeNoTextFieldBookmark ) 0x8c8c2ec 0x8c8c2b8
N2sw4mark17CheckboxFieldmarkE 0x8c8c2ec 0x8c8c2b8
N2sw4mark17CheckboxFieldmarkE
no-distro 0x8c8c2ec 0x8c8c2b8 N2sw4mark17CheckboxFieldmarkE 0x8c8c2ec
0x8c8c2b8 N2sw4mark17CheckboxFieldmarkE 0x8c8c2ec 0x8c8c2b8
N2sw4mark17CheckboxFieldmarkE reinterpret 0x8c8c2ec
N2sw4mark17CheckboxFieldmarkE
dynamic_cast( ptr ) 0x8c8c2ec
nm unxlngi6.pro/lib/libswli.so | c++filt | grep ICheckboxField
00355fc8 t sw::mark::ICheckboxFieldmark::ICheckboxFieldmark()
00356f14 t sw::mark::ICheckboxFieldmark::~ICheckboxFieldmark()
003570d0 t sw::
Re: [Libreoffice] [PATCH] fdo#34908
On Monday 28 of March 2011, Michael Meeks wrote: > On Mon, 2011-03-28 at 10:21 +0100, Noel Power wrote: > > Still I think there is something strange happening here > > Are we certain that we compiled the key method of the class (I forget > how this is calculated ;-) where the vtable is emitted with exceptions > enabled (which AFAIR enables rtti and hence enables dynamic-cast). Of > course, hopefully without that we get quite a convincing failure, rather > than a subtle one; and perhaps I'm just out of date - but could be a > factor (?) AFAIK no, -f(no-)exceptions should be completely unrelated to RTTI. -- Lubos Lunak l.lu...@suse.cz ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] fdo#34908
On Mon, 2011-03-28 at 10:21 +0100, Noel Power wrote: > Still I think there is something strange happening here Are we certain that we compiled the key method of the class (I forget how this is calculated ;-) where the vtable is emitted with exceptions enabled (which AFAIR enables rtti and hence enables dynamic-cast). Of course, hopefully without that we get quite a convincing failure, rather than a subtle one; and perhaps I'm just out of date - but could be a factor (?) ATB, Michael. -- michael.me...@novell.com <><, Pseudo Engineer, itinerant idiot ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] fdo#34908
Hi Lubos On 25/03/11 18:52, Lubos Lunak wrote: On Friday 25 of March 2011, Noel Power wrote: Ewww ... class SAL_DLLPUBLIC_EXPORT IFieldmark : virtual public IMark ... class SAL_DLLPUBLIC_EXPORT ICheckboxFieldmark : virtual public IFieldmark ... IFieldmark* pFieldmark = ... ... ICheckboxFieldmark* pCheckboxFm = reinterpret_cast(pFieldmark); You really don't want to reinterpret_cast up and down virtual inheritance. undoubtedly ( and I was uneasy about it ) but... before posting the patch I did check with valgrind and also printing out the result of the pointers from the reinterpret_cast vrs the dynamic_cast ( on both the working 64 & 32 builds ) and then valgrind again on the problematic 32bit ( with hack applied ) Does your changing from dynamic_cast to reinterpret_cast actually really fix it? Since both the classes are defined in the same place, the only reasonable explanation I see for this is that somebody got the casting similarly wrong in another place and doing it wrong here too "undoes" the first wrong. I don't have a very good explanation why this would be different for 32/64bit though. its worse as it's not just a 32bit vrs 64bit issue, its just the distro ( patches applied ) 32 that fails, all other distro/no-distro 32/64bit combos work :-/ I don't have any 32bit build, could you try with yourself few more things? First, using Valgrind is always a good idea, absolutely ( and I did that before even posting here ) and second, the output of something like this could be interesting too: printf( "%p %p %s %p %p %s %p %p %s\n", ptr, dynamic_cast< void*>( ptr ), typeid( *ptr ).name(), pFieldmark, dynamic_cast< void*>( pFieldmark ), typeid( *pFieldmark ).name(), pCheckboxFm, dynamic_cast< void*>( pCheckboxFm ), typeid( *pCheckboxFm ).name()); (where 'ptr' is what you get from the pMarksAccess->makeNoTextFieldBookmark() call before casting to anything). I'll check that later ( I didn't know about the typeid method ) so thats a nice hint. Hmm I was concentrating so much on the circumstances of the problem I missed a safer fix, the dynamic_cast/reinterpret_cast is not really necessary it's more a convenience ( to be able to call the Set/IsChecked methods ) but these methods themselves only call IFieldmark related (pure) virtual methods and no ICheckboxmark related functions ( which might someway explain why the hack works without memory foobar ) Still I think there is something strange happening here, I would suspect that you are halfway right and there is maybe somewhere a fundamental error that we get away with somehow in all cases except the distro 32 bit. But really I admit I have no clue yet. I post the results of the debug later when I get a chance, maybe it might throw some light thanks again Noel ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] fdo#34908
On Friday 25 of March 2011, Noel Power wrote: > Hi All, > I have a patch ( well really this is a workaround ) for a strange issue > ( https://bugs.freedesktop.org/show_bug.cgi?id=34908 ) that I have been > looking at on and off for the last while. > Briefly what is happening is that a dynamic_cast is failing in the > distro build ( e.g. with patches ) but only on 32 bit, the corresponding > rawbuild build works as expected. On 64, both distro and non distro > builds behave as expected ( no problems in this area ) ... > Anyway the patch/workaround is here > https://bugs.freedesktop.org/attachment.cgi?id=44788 Ewww ... class SAL_DLLPUBLIC_EXPORT IFieldmark : virtual public IMark ... class SAL_DLLPUBLIC_EXPORT ICheckboxFieldmark : virtual public IFieldmark ... IFieldmark* pFieldmark = ... ... ICheckboxFieldmark* pCheckboxFm = reinterpret_cast(pFieldmark); You really don't want to reinterpret_cast up and down virtual inheritance. Does your changing from dynamic_cast to reinterpret_cast actually really fix it? Since both the classes are defined in the same place, the only reasonable explanation I see for this is that somebody got the casting similarly wrong in another place and doing it wrong here too "undoes" the first wrong. I don't have a very good explanation why this would be different for 32/64bit though. I don't have any 32bit build, could you try with yourself few more things? First, using Valgrind is always a good idea, and second, the output of something like this could be interesting too: printf( "%p %p %s %p %p %s %p %p %s\n", ptr, dynamic_cast< void* >( ptr ), typeid( *ptr ).name(), pFieldmark, dynamic_cast< void* >( pFieldmark ), typeid( *pFieldmark ).name(), pCheckboxFm, dynamic_cast< void* >( pCheckboxFm ), typeid( *pCheckboxFm ).name()); (where 'ptr' is what you get from the pMarksAccess->makeNoTextFieldBookmark() call before casting to anything). -- Lubos Lunak l.lu...@suse.cz ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
[Libreoffice] [PATCH] fdo#34908
Hi All, I have a patch ( well really this is a workaround ) for a strange issue ( https://bugs.freedesktop.org/show_bug.cgi?id=34908 ) that I have been looking at on and off for the last while. Briefly what is happening is that a dynamic_cast is failing in the distro build ( e.g. with patches ) but only on 32 bit, the corresponding rawbuild build works as expected. On 64, both distro and non distro builds behave as expected ( no problems in this area ) Note: nm -C | grep CheckboxMark on the various libraries yields afaics mostly identical results, another test I did was to build the sw module from the rawbuild tree in the distro tree, running with those libraries yields the same problem ( confirming what I already thought from lots of manual patch reverts ) but interestingly moving those same libraries into the the rawbuild install and they work again :-/ Anyway the patch/workaround is here https://bugs.freedesktop.org/attachment.cgi?id=44788 In addition to review it would be great to get some opinion on what is the best way to deliver this fix, myself and Petr couldn't quite decide which was the best option, the choices are a) leave this as a patch ( with some note in apply ) at least we wont forget about the problem b) commit directly into the branch, this sortof sweeps the problem under the carpet ( which make me a little uncomfortable ) Noel ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] fdo #34908
> I'd like to cherry pick that to libreoffice3.3 Looks alright to me. --tml ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
[Libreoffice] [PATCH] fdo #34908
Hi http://cgit.freedesktop.org/libreoffice/writer/commit/?id=636fa8da77e438fa04d331d96c4a8ebc5317560f I'd like to cherry pick that to libreoffice3.3 thanks Noel ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
