when I debug MacOS X hebrew support with smontagu, we find out one major problem with bidi code. The nsFrame::GetBidiProperty and SetBidiProperty pair is bad for Big Endian machinese: http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsFrame.cpp#4104 NS_IMETHODIMP nsFrame::GetBidiProperty(nsIPresContext* aPresContext, nsIAtom* aPropertyName, void** aPropertyValue, size_t aSize) const { if (!aPropertyValue || !aPropertyName) { return NS_ERROR_NULL_POINTER; } if ( (aSize < 1) || (aSize > sizeof(void*) ) ) { return NS_ERROR_ILLEGAL_VALUE; } nsCRT::zero(aPropertyValue, aSize); void* val = nsnull; nsCOMPtr<nsIPresShell> presShell; aPresContext->GetShell(getter_AddRefs(presShell) ); if (presShell) { nsCOMPtr<nsIFrameManager> frameManager; presShell->GetFrameManager(getter_AddRefs(frameManager) ); if (frameManager) { frameManager->GetFrameProperty( (nsIFrame*)this, aPropertyName, 0, &val); if (val) { nsCRT::memcpy(aPropertyValue, &val, aSize); } } } return NS_OK; } NS_IMETHODIMP nsFrame::SetBidiProperty(nsIPresContext* aPresContext, nsIAtom* aPropertyName, void* aPropertyValue) { nsresult rv = NS_ERROR_FAILURE; nsCOMPtr<nsIPresShell> shell; aPresContext->GetShell(getter_AddRefs(shell) ); if (shell) { nsCOMPtr<nsIFrameManager> frameManager; shell->GetFrameManager(getter_AddRefs(frameManager) ); if (frameManager) { rv = frameManager->SetFrameProperty( (nsIFrame*) this, aPropertyName, aPropertyValue, nsnull); } } return rv; } What happen is when the aSize is < 4 and in the big endian machine. The SetProperty will just copy size of (void*) to the hash table. But the GetBidiProperty will copy the beginning aSize and return. In the little endian machine, it is not a big deal. But if the return value is 0x00000001 and on big endian machine and ask for 1 byte return. The GetBidiProperty will copy the 00 return instead of 0x01 return. Therefore, on MacOS, the embedding level is always 0.

I think this will always cause problem on other machines, such as AIX or Solaris. As long as it is big endian machine.

reassign to self

ok. this patch compiled and run fine on MacOS X compiled on Win and Linux. (not run time change for little endian macine) mkaply can kataki, can you try this patch on AIX and big endian Solaris?

mkply: can r= this one ?

shanjian: my patch let compiler do the magic since it is the compiler which cast the PRInt8 it void* eariler. Your patch probably will also work under big endian. What will happen to 64 bits machine ?

shanjian send the following thorugh private email: >Frank, > >I tried this on my HPUX box, and it does not compile. HPUX does not allow hard >cast from void* to PRUint8 and PRUint16. I modified the patch and attached to bug >report to save a round of email.

Comment on attachment 62884 [details] [diff] [review] modified patch The SetBidiProperty interface is broken. It should take an aSize parameter and a *pointer* to an integral type, and copy that many bytes from the given address into whatever intermediate or internal buffer it needs (in this case, a void* that's passed to SetFrameProperty). As a quick fix, shanjian's patch is ok, but I'd rather see the interface methods made symmetric. There are only a few file's worth of a few calls per file to SetBidiProperty. /be

I believe the following three patch should fix most mac os hewbrew problem big endian issue in nsFrame.cpp see latest patch in http://bugzilla.mozilla.org/show_bug.cgi?id=116976 big endian issue in nsBidiImp.cpp http://bugzilla.mozilla.org/attachment.cgi?id=62917&action=view turn off hebrew reordering and do not report hint on mac gfx http://bugzilla.mozilla.org/attachment.cgi?id=62920&action=view

What are the different BiDi properties that are being read and set here? Are they always 32 bits or less? Why not just get/set 32-bit values, rather than void*, and do the up/down cast in the calling code?

it could be a nsIFrame*. It could be > 32 bits on a 64 bits machine. Basically, I agree with you, I think we could always get a (void*) , not a 32 bits. and let the caller cast it down to what ever it shoudl be. Let's check in shanjian's patch and fix the api on a seperate bug. see bug 117751

could we check in http://bugzilla.mozilla.org/attachment.cgi?bugid=116976&action=enter for now and fix the api issue in bug 117751?

Comment on attachment 62884 [details] [diff] [review] modified patch r=ftang

jdunn, can you try to compile on those big endian machines you have ?

kataki- do you have big endian machine (solaris?)

the patch compiled on Solaris w/o problem

I can test this on Solaris/SPARC if someone can explain me how I can verify this ... BTW: Why are we using nsCRT::memcpy() instead of ANSI C memcpy() (this destroys the optimisations some compiler/OSes can do when using their "built-in" memcpy() versions) ?

It compiled on HPUX

>BTW: Why are we using nsCRT::memcpy() instead of ANSI C memcpy() (this destroys >the optimisations some compiler/OSes can do when using their "built-in" memcpy() >versions) ? We should always use nsCRT if possible in xp code. If Compiler/OS can do with their build-in memcpy() can we fix that in nsCRT::memcpy() ?

>I can test this on Solaris/SPARC if someone can explain me how I can verify this thanks, I already verify it on Solaris. It should impact the correctness of Hebrew/Arabic display . Hard to explain.

ftang wrote: > We should always use nsCRT if possible in xp code. If Compiler/OS can do with > their build-in memcpy() can we fix that in nsCRT::memcpy() ? Not really. The built-in compiler memcpy() requires to be |inline| because they look for common optimisations (small fixed[1] size (inline long word block copies), large fixed size (which calls specfic OS routine for large mem copies, alignment optimisations etc. etc) [1]=("fixed" = either constant fixed value or the compiler can figure out that the variable may only contain a known value/expresson which can be used for further optimisation). I would vote to implement a PL_memcpy() which should be a macro to memcpy() on platforms where memcpy() is not available (is there any ?) ...

I don't believe we need PL_strcpy, nsCRT::memcpy, etc. any longer. The rationale for these is almost lost in the mists of time (I dimly recall an HP-UX memcpy bug from 1996). jdunn, do you agree? I see lots of Mozilla code using memcpy, strlen, etc. and porting as well as any other code. I see on reason to rub these hold-over "portability worry beads" any longer. Sorry I didn't say this earlier. /be

I can't recall the memcpy issue from '96. The lastest patch does look ok, however you should get Shanmu to look at this on Tru64 (unless someone else has done a 64bit build?!?) However, it looks ok to me. Shanmu, can you try to make sure the latest patch compiles on Tru64? I have no idea how to test it... I also agree with Brendan maybe it is time we go thru and look at removing nsCRT::memcpy's but if we do, we should do that as a separate issue since it is going to consume a bunch of time (I assume we will do performance analysis on it as well)

Comment on attachment 63501 [details] [diff] [review] updated patch ues ::memcpy directly r=Roland.Mainz@informatik.med.uni-giessen.de

I am confused. What is the significance of using ::memcpy and NOT memcpy?

|::memcpy()| requests explicitly a "_mempy" symbol from the (flat) ANSI-C namespace. |memcpy| can be anything... function, macro... what you want (or what the compiler can offer... :) ...

The latest patch compiled on a Tru64 UNIX machine.

jdunn: agreed, we can leave existing nsCRT::memcpy usage, but I don't see any reason to add new uses. sr=brendan@mozilla.org contingent on successful Tru64 testing (code looks good to me, but test platform coverage is key here). /be

Ok, but for the record I don't want to keep ANY instances of nsCRT::memcpy around. Lets open another bug and use that one to clean them up (I think that is the consensus.

Comment on attachment 63501 [details] [diff] [review] updated patch ues ::memcpy directly jdunn: sorry I was unclear, I meant we don't need to hassle with nsCRT::memcpy=>memcpy in existing code touched by this or another bug that's already on file, we can use the new bug ftang kindly filed to do the cleanup, as you say. ftang: thanks for filing that! sr=brendan@mozilla.org. /be

jdunn: ::memcpy is pure C++ for "call the standard memcpy in the global namespace, which must conform to the standard." Calling memcpy (no ::) might call some other method if we had a default namespace that was not the global or standard one, or if someone added memcpy to the class whose method contains the call, or whatever. Since it would be evil, and prolly break lots of code, for anyone to name any method memcpy other than the standard global function, this is more a purity-of-essence or style issue. It's ok with me to call memcpy either way, but I defer to the C++ experts (cc'ing dbaron). /be

Roland: ::memcpy does *not* explicitly request a _memcpy symbol from anywhere, AFAIK. I believe you are confusing linker implementation details on Unix-like systems with the C++ standard. Please correct me if I am wrong with something from the standard, and (better) with real-world compilers that treat ::memcpy differently from memcpy (not that macros expand even when their calls are preceded by punctuation such as ::). /be

let's keep the memcpy discussion on bug 118135 please.

fixed and check in.

according to the discuss of bug 118135 the fix can change ::memcpy to memcpy

In my solaris platform, I apply this patch, it works . but when I change the patch as below #if IS_BIG_ENDIAN nsCRT::memcpy(aPropertyValue, ((char*)&val)+sizeof(void*) - aSize, aSize); #else nsCRT::memcpy(aPropertyValue, &val, aSize); #endif it works too. So I wonder should we change all nsCRT::memcpy to memcpy? Is it useful?

Patch works for me on Solaris.