Closed Bug 1396850 Opened 7 years ago Closed 7 years ago

clang -Wundefined-var-template warnings in JNI and widget code

Categories

(Core Graveyard :: Widget: Android, defect)

All
Android
defect
Not set
normal

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(1 file, 1 obsolete file)

clang complains thusly:

In file included from /opt/build/froydnj/build-android/widget/android/Unified_cpp_widget_android1.cpp:47:
In file included from /home/froydnj/src/gecko-dev.git/widget/android/nsWidgetFactory.cpp:14:
/home/froydnj/src/gecko-dev.git/widget/android/nsWindow.h:136:44: warning: instantiation of variable 'nsWindow::NativePtr<mozilla::widget::GeckoEditableSupport>::sName' required here, but no definition is available [-Wundefined-var-template]
            , mWindowLock(NativePtr<Impl>::sName)
                                           ^
/home/froydnj/src/gecko-dev.git/widget/android/GeckoEditableSupport.h:173:11: note: in instantiation of member function 'nsWindow::WindowPtr<mozilla::widget::GeckoEditableSupport>::WindowPtr' requested here
        , mWindow(aPtr, aWindow)
          ^
/home/froydnj/src/gecko-dev.git/widget/android/nsWindow.h:84:27: note: forward declaration of template entity is here
        static const char sName[];
                          ^
/home/froydnj/src/gecko-dev.git/widget/android/nsWindow.h:136:44: note: add an explicit instantiation declaration to suppress this warning if 'nsWindow::NativePtr<mozilla::widget::GeckoEditableSupport>::sName' is explicitly instantiated in another translation unit
            , mWindowLock(NativePtr<Impl>::sName)
                                           ^
In file included from /opt/build/froydnj/build-android/widget/android/Unified_cpp_widget_android0.cpp:2:
In file included from /home/froydnj/src/gecko-dev.git/widget/android/ANRReporter.cpp:6:
In file included from /home/froydnj/src/gecko-dev.git/widget/android/ANRReporter.h:9:
In file included from /home/froydnj/src/gecko-dev.git/widget/android/fennec/FennecJNINatives.h:10:
In file included from /opt/build/froydnj/build-android/dist/include/FennecJNIWrappers.h:10:
/opt/build/froydnj/build-android/dist/include/mozilla/jni/Refs.h:245:55: warning: instantiation of variable 'mozilla::jni::ObjectBase<mozilla::jni::TypedObject<jstring>, _jstring *>::name' required here, but no definition is available [-Wundefined-var-template]
            const jclass cls = GetClassRef(mEnv, Cls::name);
                                                      ^
/opt/build/froydnj/build-android/dist/include/mozilla/jni/Refs.h:957:53: note: in instantiation of member function 'mozilla::jni::Context<mozilla::jni::TypedObject<jstring>, _jstring *>::ClassRef' requested here
                typename Cls::Context(env, nullptr).ClassRef(),
                                                    ^
/home/froydnj/src/gecko-dev.git/widget/android/EventDispatcher.cpp:228:35: note: in instantiation of function template specialization 'mozilla::jni::TypedObject<_jobjectArray *>::New<mozilla::jni::TypedObject<jstring> >' requested here
    auto keys = jni::ObjectArray::New<jni::String>(length);
                                  ^
/opt/build/froydnj/build-android/dist/include/mozilla/jni/Refs.h:317:23: note: forward declaration of template entity is here
    static const char name[];
                      ^
/opt/build/froydnj/build-android/dist/include/mozilla/jni/Refs.h:245:55: note: add an explicit instantiation declaration to suppress this warning if 'mozilla::jni::ObjectBase<mozilla::jni::TypedObject<jstring>, _jstring *>::name' is explicitly instantiated in another translation unit
            const jclass cls = GetClassRef(mEnv, Cls::name);

I tried to rewrite the first bit of problematic code, but I didn't see a good way to resolve the circular dependence between NativePtr and WindowPtr.  And the second one defeats me completely.  Jim, are you able to produce patches that can fix the problem these warnings are pointing out?
Flags: needinfo?(nchen)
I think for the first warning, we should move the definition at [1] to inside nsWindow.cpp, where we instantiate other instances of `sName` (e.g. [2])

[1] http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/widget/android/GeckoEditableSupport.cpp#29
[2] http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/widget/android/nsWindow.cpp#736

For the second warning, I think the proper fix is to copy the `name[]` definitions from [3] to Ref.h, and make them declarations. For example, somewhere around [4], we would have a list of

> template<> const char ObjectBase<Object, jobject>::name[];
> template<> const char ObjectBase<TypedObject<jstring>, jstring>::name[];
> ...

[3] http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/widget/android/jni/Utils.cpp#52
[4] http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/widget/android/jni/Refs.h#354

Let me know if you want me to write the patch, though I think you are in a better position to test if the patch actually works or not :)
Flags: needinfo?(nchen)
(In reply to Jim Chen [:jchen] [:darchons] from comment #1)
> I think for the first warning, we should move the definition at [1] to
> inside nsWindow.cpp, where we instantiate other instances of `sName` (e.g.
> [2])
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/widget/android/GeckoEditableSupport.
> cpp#29
> [2]
> http://searchfox.org/mozilla-central/rev/
> f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/widget/android/nsWindow.cpp#736

It's not clear to me why this would work.  The error messages suggest that the errors are coming from the headers themselves, without any .cpp involvement.

> For the second warning, I think the proper fix is to copy the `name[]`
> definitions from [3] to Ref.h, and make them declarations. For example,
> somewhere around [4], we would have a list of
> 
> > template<> const char ObjectBase<Object, jobject>::name[];
> > template<> const char ObjectBase<TypedObject<jstring>, jstring>::name[];
> > ...
> 
> [3]
> http://searchfox.org/mozilla-central/rev/
> f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/widget/android/jni/Utils.cpp#52
> [4]
> http://searchfox.org/mozilla-central/rev/
> f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/widget/android/jni/Refs.h#354

I will have to think about this; we don't want the definitions in the header itself, certainly.
Hm I think you're right. IIUC the warning happens when we try to instantiate `nsWindow` inside nsWidgetFactory.cpp (and therefore `nsWindow::NativePtr<GeckoEditableSupport>::sName`), but `sName` is not declared within nsWidgetFactory's translation unit (note that we declare the templated `sName` but not the instantiated `sName`).

So the solution is to somehow declare the instantiated `sName` in nsWidgetFactory's translation unit. We can add declarations to nsWindow.h, or we can add definitions to nsWidgetFactory.cpp. Either way should work.
Doing this avoids -Wundefined-var-template warnings with clang.  This
warning is mostly harmless (clang is trying to tell you a linker error
might be awaiting you later), but being careful with this might make
using C++ modules easier somewhere down the line.
Attachment #8904798 - Flags: review?(nchen)
Comment on attachment 8904798 [details] [diff] [review]
explicitly declare static members of templates prior to use

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

::: build/moz.configure/warnings.configure
@@ +92,5 @@
>  # we don't want our builds held hostage when a platform-specific API
>  # becomes deprecated.
>  check_and_add_gcc_warning('-Wno-error=deprecated-declarations')
>  
> +#check_and_add_gcc_warning('-Wno-error=undefined-var-template')

Argh, ignore this part, sorry. :(
Comment on attachment 8904798 [details] [diff] [review]
explicitly declare static members of templates prior to use

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

::: widget/android/jni/Refs.h
@@ +313,5 @@
>      using GlobalRef = jni::GlobalRef<Cls>;
>      using Param = const Ref&;
>  
>      static const CallingThread callingThread = CallingThread::ANY;
> +    static const char* const name;

The declarations should be enough? Is it necessary to change the type here? You would need to change all the types in GeneratedJNIWrappers/FennecJNIWrappers, and the annotation processor as well.

::: widget/android/nsWindow.h
@@ -132,5 @@
>  
> -        WindowPtr(NativePtr<Impl>* aPtr, nsWindow* aWindow)
> -            : mPtr(aPtr)
> -            , mWindow(aWindow)
> -            , mWindowLock(NativePtr<Impl>::sName)

Might be more clear to add `NativePtr<Impl>::GetName()` and use that, then I think you can keep `WindowPtr::WindowPtr` inlined.
(In reply to Jim Chen [:jchen] [:darchons] from comment #6)
> ::: widget/android/jni/Refs.h
> @@ +313,5 @@
> >      using GlobalRef = jni::GlobalRef<Cls>;
> >      using Param = const Ref&;
> >  
> >      static const CallingThread callingThread = CallingThread::ANY;
> > +    static const char* const name;
> 
> The declarations should be enough? Is it necessary to change the type here?
> You would need to change all the types in
> GeneratedJNIWrappers/FennecJNIWrappers, and the annotation processor as well.

Ugh, probably didn't notice that because I was running a debug build.

What "declarations should be enough" are you referring to?

> ::: widget/android/nsWindow.h
> @@ -132,5 @@
> >  
> > -        WindowPtr(NativePtr<Impl>* aPtr, nsWindow* aWindow)
> > -            : mPtr(aPtr)
> > -            , mWindow(aWindow)
> > -            , mWindowLock(NativePtr<Impl>::sName)
> 
> Might be more clear to add `NativePtr<Impl>::GetName()` and use that, then I
> think you can keep `WindowPtr::WindowPtr` inlined.

Accessing `sName` in `GetName()` has the same issue with regards to the warning.
Let's try this version.  Botond helped me get the template declarations right,
so we don't have to change any of the datatypes involved.
Attachment #8904806 - Flags: review?(nchen)
Attachment #8904798 - Attachment is obsolete: true
Attachment #8904798 - Flags: review?(nchen)
Assignee: nobody → nfroyd
Comment on attachment 8904806 [details] [diff] [review]
explicitly declare static members of templates prior to use

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

::: widget/android/jni/Refs.h
@@ +374,5 @@
> +template<> const char ObjectBase<TypedObject<jobjectArray>, jobjectArray>::name[];
> +
> +class ByteBuffer;
> +
> +template<> const char ObjectBase<ByteBuffer, jobject>::name[];

I would move this to right after where we define `ByteBuffer` below, but nbd.

::: widget/android/nsWindow.h
@@ +354,5 @@
> +template<> const char nsWindow::NativePtr<mozilla::widget::GeckoEditableSupport>::sName[];
> +template<> const char nsWindow::NativePtr<nsWindow::NPZCSupport>::sName[];
> +
> +template<class Impl>
> +nsWindow::WindowPtr<Impl>::WindowPtr(NativePtr<Impl>* aPtr, nsWindow* aWindow)

> Accessing `sName` in `GetName()` has the same issue with regards to the warning.

So I meant adding `NativePtr::GetName()` and putting its definition down here instead of `nsWindow::WindowPtr`, so `nsWindow::WindowPtr` can stay inlined, but nbd either.
Attachment #8904806 - Flags: review?(nchen) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3dd3dbdc735
explicitly declare static members of templates prior to use; r=darchons
https://hg.mozilla.org/mozilla-central/rev/e3dd3dbdc735
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.