Closed Bug 815581 Opened 7 years ago Closed 7 years ago

Allow compiling accessibility on mingw

Categories

(Core :: Disability Access APIs, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: jacek, Assigned: jacek)

References

Details

Attachments

(3 files, 1 obsolete file)

Accessibility was always disabled for mingw due to lack of MIDL replacement. Wine project, however, has implemented a replacement called widl for a long time. I've integrated widl into mingw-w64 project so that now it's an optional tool. Widl itself required a few minor fixes to be drop-in replacement for Mozilla and all of them are upstreamed now. I will attach patches that are enough for successful mingw compilation of accessibility (it's all still experimental and not really tested in runtime yet). I will update
https://developer.mozilla.org/en-US/docs/Cross_Compile_Mozilla_for_Mingw32
with instructions for widl compilation once these patches land (it's a matter of compiling and installing widl from mingw-w64-tools/widl).
Attached patch makefile part (obsolete) — Splinter Review
Not much changes in makefiles. configure.in changes just looks for widl on the path. It also disables accessibility automatically if widl is not found. This will fix bug 341965.
Attachment #685560 - Flags: review?(ted)
I separated this patch from other fixes due to its size. GCC doesn't support SEH (well, it does to some degree on x86_64 now, but still no __try/__except syntax). We have MOZ_SEH_TRY/MOZ_SEH_EXCEPT to deal with it in most cases. For accessibility, there were already another macros A11Y_TRYBLOCK_BEGIN/A11Y_TRYBLOCK_END that make the code cleaner IMHO, but they were used in just a few places. I changed the code to use them everywhere instead of __try/__except. Note that this patch is not no-op. Macro A11Y_TRYBLOCK_END always returns failure in case of an exception, while in some cases previous code returned success. IMHO a failure is the right thing to do in those cases.
Attachment #685565 - Flags: review?(trev.saunders)
This part contains remaining compilation fixes. They are:

- Fixed includes on case-sensitive systems
- Include Accessible-inl.h where it's used (it's more critical than usually because we use -fno-keep-inline-dllexport on mingw)
- Fixed the cast losing precision in GetRuntimeId in 64-bit compilation. It still loses precision (which is fine in this case), but not in the direct pointer cast, but instead in implicit integer cast.
- I also silenced the very verbose warning. I will look at other GCC warnings once this bug lands
Attachment #685567 - Flags: review?(trev.saunders)
Duplicate of this bug: 203296
Try is green.
(In reply to Jacek Caban from comment #2)
> Created attachment 685565 [details] [diff] [review]
> Replace explicit SEH blocks by macros
> 
> I separated this patch from other fixes due to its size. GCC doesn't support
> SEH (well, it does to some degree on x86_64 now, but still no __try/__except
> syntax). We have MOZ_SEH_TRY/MOZ_SEH_EXCEPT to deal with it in most cases.
> For accessibility, there were already another macros
> A11Y_TRYBLOCK_BEGIN/A11Y_TRYBLOCK_END that make the code cleaner IMHO, but
> they were used in just a few places. I changed the code to use them
> everywhere instead of __try/__except. Note that this patch is not no-op.

yeah, the macros are sort of new but we've been replacing stuff with them as we touch it and it makes sense.  Thankyou for finishing this.

> Macro A11Y_TRYBLOCK_END always returns failure in case of an exception,
> while in some cases previous code returned success. IMHO a failure is the
> right thing to do in those cases.

agreed
Comment on attachment 685567 [details] [diff] [review]
compilation fixes

>-  *uniqueID = - reinterpret_cast<long>(UniqueID());
>+  *uniqueID = - reinterpret_cast<LONG_PTR>(UniqueID());

nit, intptr_t might be a nicer thing to cast to, but I'm not absolutely clear on what long_ptr is.

>+#ifdef __GNUC__
>+// Inheriting from both XPCOM and MSCOM interfaces causes a lot of warnings
>+// about virtual functions being hidden by each other. This is done by
>+// design, so silence the warning.

its really more a legacy thing we want to fix, but this seems fine.

>-  int ids[] = { UiaAppendRuntimeId, reinterpret_cast<int>(mAcc->UniqueID()) };
>+  int ids[] = { UiaAppendRuntimeId, reinterpret_cast<LONG_PTR>(mAcc->UniqueID()) };

same comment on LONG_PTR
Attachment #685567 - Flags: review?(trev.saunders) → review+
Comment on attachment 685565 [details] [diff] [review]
Replace explicit SEH blocks by macros

A11Y_TRYBLOCK_END                                                      \
>-  } __except(nsAccessNodeWrap::FilterA11yExceptions(::GetExceptionCode(),      \
>+  } MOZ_SEH_EXCEPT(nsAccessNodeWrap::FilterA11yExceptions(::GetExceptionCode(),\
>                                                     GetExceptionInformation()))\

nit fix the alignment here
Attachment #685565 - Flags: review?(trev.saunders) → review+
(In reply to Trevor Saunders (:tbsaunde) from comment #8)
> Comment on attachment 685567 [details] [diff] [review]
> compilation fixes
> 
> >-  *uniqueID = - reinterpret_cast<long>(UniqueID());
> >+  *uniqueID = - reinterpret_cast<LONG_PTR>(UniqueID());

that must be wrong because *uniqueID is not long*, it's long.

> >-  int ids[] = { UiaAppendRuntimeId, reinterpret_cast<int>(mAcc->UniqueID()) };
> >+  int ids[] = { UiaAppendRuntimeId, reinterpret_cast<LONG_PTR>(mAcc->UniqueID()) };

same issue here
(In reply to alexander :surkov from comment #10)
> (In reply to Trevor Saunders (:tbsaunde) from comment #8)
> > Comment on attachment 685567 [details] [diff] [review]
> > compilation fixes
> > 
> > >-  *uniqueID = - reinterpret_cast<long>(UniqueID());
> > >+  *uniqueID = - reinterpret_cast<LONG_PTR>(UniqueID());
> 
> that must be wrong because *uniqueID is not long*, it's long.

http://msdn.microsoft.com/en-us/library/cc230349%28v=prot.20%29.aspx says LONG_PTR is   windows(intptr_t)
Comment on attachment 685565 [details] [diff] [review]
Replace explicit SEH blocks by macros

Review of attachment 685565 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/msaa/AccessibleWrap.cpp
@@ +830,4 @@
>      return S_OK;
>    }
>  
> +  A11Y_TRYBLOCK_END

I prefer to have explicit return

::: accessible/src/msaa/nsAccessNodeWrap.cpp
@@ +252,2 @@
>  }
>          

nit: it'd be nice if you removed whitespaces while you are here
(In reply to Trevor Saunders (:tbsaunde) from comment #11)

> > > >-  *uniqueID = - reinterpret_cast<long>(UniqueID());
> > > >+  *uniqueID = - reinterpret_cast<LONG_PTR>(UniqueID());
> > 
> > that must be wrong because *uniqueID is not long*, it's long.
> 
> http://msdn.microsoft.com/en-us/library/cc230349%28v=prot.20%29.aspx says
> LONG_PTR is   windows(intptr_t)

yep, I figured it out, thanks :) That ptr thing is confusing.
(In reply to alexander :surkov from comment #13)
> (In reply to Trevor Saunders (:tbsaunde) from comment #11)
> 
> > > > >-  *uniqueID = - reinterpret_cast<long>(UniqueID());
> > > > >+  *uniqueID = - reinterpret_cast<LONG_PTR>(UniqueID());
> > > 
> > > that must be wrong because *uniqueID is not long*, it's long.
> > 
> > http://msdn.microsoft.com/en-us/library/cc230349%28v=prot.20%29.aspx says
> > LONG_PTR is   windows(intptr_t)
> 
> yep, I figured it out, thanks :) That ptr thing is confusing.

this one describes (http://msdn.microsoft.com/en-us/library/cc230349%28v=prot.20%29.aspx) it as typedef __int3264 LONG_PTR; and it seems clearer.
(In reply to alexander :surkov from comment #14)
> (In reply to alexander :surkov from comment #13)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #11)
> > 
> > > > > >-  *uniqueID = - reinterpret_cast<long>(UniqueID());
> > > > > >+  *uniqueID = - reinterpret_cast<LONG_PTR>(UniqueID());
> > > > 
> > > > that must be wrong because *uniqueID is not long*, it's long.
> > > 
> > > http://msdn.microsoft.com/en-us/library/cc230349%28v=prot.20%29.aspx says
> > > LONG_PTR is   windows(intptr_t)
> > 
> > yep, I figured it out, thanks :) That ptr thing is confusing.
> 
> this one describes
> (http://msdn.microsoft.com/en-us/library/cc230349%28v=prot.20%29.aspx) it as
> typedef __int3264 LONG_PTR; and it seems clearer.

__int3264 is madness too shall we go with intptr_t since we can, and its a sensible type for what we want?
they say: "A LONG_PTR is a long type used for pointer precision. It is used when casting a pointer to a long type to perform pointer arithmetic." Since we are in Windows world then it should be a right way. Maybe that was a reason why Jacek used it.
Thanks for reviews.

> >-  } __except(nsAccessNodeWrap::FilterA11yExceptions(::GetExceptionCode(),      \
> >+  } MOZ_SEH_EXCEPT(nsAccessNodeWrap::FilterA11yExceptions(::GetExceptionCode(),\
> >                                                     GetExceptionInformation()))\
>
> nit fix the alignment here

Fixed.

> ::: accessible/src/msaa/AccessibleWrap.cpp
> @@ +830,4 @@
> >      return S_OK;
> >    }
> >  
> > +  A11Y_TRYBLOCK_END
>
> I prefer to have explicit return

Agreed, fixed.

> ::: accessible/src/msaa/nsAccessNodeWrap.cpp
> @@ +252,2 @@
> >  }
> >          
>
> nit: it'd be nice if you removed whitespaces while you are here

I couldn't figure from this quote which whitespaces you meant, but after bug 767756 landed (yesterday) I had to rebase my patch and it doesn't touch this file anymore.

> they say: "A LONG_PTR is a long type used for pointer precision. It is used when casting a > pointer to a long type to perform pointer arithmetic." Since we are in Windows world then > it should be a right way. Maybe that was a reason why Jacek used it.

Yes, the reason I used it instead of intptr_t is probably my habit of using it in Windows world taken from Wine development. Since it caused some confusion, it's clear to me that intptr_t is better in this case.

BTW, when changing it, I realized that my change in uiaRawElmProvider::GetRuntimeId was still causing warning on 64-bit build (as oposed to original error), because implicit intptr_t->int casts are not enough for GCC in array initializers. I added one more static_cast there, I hope it's OK.
Comment on attachment 685560 [details] [diff] [review]
makefile part

Review of attachment 685560 [details] [diff] [review]:
-----------------------------------------------------------------

This is mostly fine, just one issue.

::: configure.in
@@ +739,5 @@
> +                MIDL_FLAGS="$MIDL_FLAGS --win64 -m64"
> +                ;;
> +            esac
> +        else
> +            ACCESSIBILITY=

Our current rule of thumb is to not silently disable features when prerequisites are missing, but to instead error about them, including the configure option that would let you continue building (--disable-accessibility here), and the prerequisites that are missing (and how to install them, if that's simple).
Attachment #685560 - Flags: review?(ted) → review-
I also added an error for MSVC compilation without MIDL for consistency.
Attachment #685560 - Attachment is obsolete: true
Attachment #687018 - Flags: review?(ted)
Comment on attachment 687018 [details] [diff] [review]
makefile part v1.1

Review of attachment 687018 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, that's perfect!
Attachment #687018 - Flags: review?(ted) → review+
Thatnks for the review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c581bb3ceac5
Whiteboard: leave open
And bustage fix for non-Windows platforms:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d2bb4d470cc

The MIDL check should be done only on Windows platform, sorry about that.
...and a typo fix, spotted by edmorley:

https://hg.mozilla.org/integration/mozilla-inbound/rev/41723231257e
You need to log in before you can comment on or make changes to this bug.