815581 - Allow compiling accessibility on mingw

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Jacek Caban]

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).

Comment 1

[Jacek Caban]

Attachment: makefile part

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.

Comment 2

[Jacek Caban]

Attachment: 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. 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.

Comment 3

[Jacek Caban]

Attachment: compilation fixes

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

Comment 4

[Jacek Caban]

Try commit: https://tbpl.mozilla.org/?tree=Try&rev=b6e1190f02cb

Comment 6

[Jacek Caban]

Try is green.

Comment 7

[Trevor Saunders (:tbsaunde)]

(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 8

[Trevor Saunders (:tbsaunde)]

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

Comment 9

[Trevor Saunders (:tbsaunde)]

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

Comment 10

[alexander :surkov (:asurkov)]

(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

Comment 11

[Trevor Saunders (:tbsaunde)]

(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 12

[alexander :surkov (:asurkov)]

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

Comment 13

[alexander :surkov (:asurkov)]

(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.

Comment 14

[alexander :surkov (:asurkov)]

(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.

Comment 15

[Trevor Saunders (:tbsaunde)]

(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?

Comment 16

[alexander :surkov (:asurkov)]

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.

Comment 17

[Jacek Caban]

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 18

[Jacek Caban]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ddabcd6f9491
https://hg.mozilla.org/integration/mozilla-inbound/rev/476e02f9a619

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ddabcd6f9491
https://hg.mozilla.org/mozilla-central/rev/476e02f9a619

Comment 20

[(not currently active) Ted Mielczarek]

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).

Comment 21

[Jacek Caban]

Attachment: makefile part v1.1

I also added an error for MSVC compilation without MIDL for consistency.

Comment 22

[(not currently active) Ted Mielczarek]

Comment on attachment 687018 [details] [diff] [review]
makefile part v1.1

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

Thanks, that's perfect!

Comment 23

[Jacek Caban]

Thatnks for the review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c581bb3ceac5

Comment 24

[Jacek Caban]

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.

Comment 25

[Jacek Caban]

...and a typo fix, spotted by edmorley:

https://hg.mozilla.org/integration/mozilla-inbound/rev/41723231257e

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c581bb3ceac5
https://hg.mozilla.org/mozilla-central/rev/4d2bb4d470cc
https://hg.mozilla.org/mozilla-central/rev/41723231257e