1277650 - make GeneratedJNI.h wrappers work with clang

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Nathan Froyd [:froydnj]]

clang gives errors like:

In file included from /home/froydnj/src/gecko-dev.git/dom/mobilemessage/android/SmsManager.cpp:6:
In file included from /home/froydnj/src/gecko-dev.git/dom/mobilemessage/android/SmsManager.h:9:
/home/froydnj/src/gecko-dev.git/widget/android/GeneratedJNINatives.h:135:40: error: no member named 'NotifyCursorDone' in 'mozilla::SmsManager'
                ::template Wrap<&Impl::NotifyCursorDone>),
                                 ~~~~~~^
/home/froydnj/src/gecko-dev.git/dom/mobilemessage/android/SmsManager.h:13:27: note: in instantiation of template class 'mozilla::widget::GeckoSmsManager::Natives<mozilla::SmsManager>' requested here
class SmsManager : public widget::GeckoSmsManager::Natives<SmsManager>

And SmsManager, of course, has a NotifyCursorDone method:

http://dxr.mozilla.org/mozilla-central/source/dom/mobilemessage/android/SmsManager.cpp#385

I feel like I have run into this problem before, but I don't remember what the resolution was.  Or maybe this was a bug in clang that was supposed to be fixed in subsequent releases, but the fix hasn't made it into the NDK yet?  Do you remember?

Comment 1

[Nathan Froyd [:froydnj]]

See bug 1163171 comment 4.  Guess this was an old clang problem after all.

Comment 2

[Nathan Froyd [:froydnj]]

OK, multiple versions of clang complain about essentially the problem demonstrated below:

#include <inttypes.h>

typedef struct {
    const char* name;
    const char* signature;
    void*       fnPtr;
} JNINativeMethod;

class GeckoSmsManager
{
public:
  template<class Impl> class Natives;
};

template<class Impl>
class GeckoSmsManager::Natives
{
public:
  static constexpr JNINativeMethod methods[] = {
    { "name", "signature", reinterpret_cast<void*>(&Impl::NotifyCursorDone) }
  };
};

template<class Impl>
constexpr JNINativeMethod GeckoSmsManager::Natives<Impl>::methods[];

class SmsManager : public GeckoSmsManager::Natives<SmsManager>
{
public:
  static void NotifyCursorDone(int32_t);
};

They complain that SmsManager doesn't have any member named "NotifyCursorDone", when it clearly does.  But GCC, MSVC, and icc (icc checked with gcc.godbolt.org) all compile it without a fuss.  Is clang correct here, or is this a bug that needs to be fixed?  (I haven't checked with the most-up-to-date clang yet, so it's possible it *is* fixed...)

Comment 3

[Botond Ballo [:botond]]

I can make a pretty good guess as to why clang is giving the error:

  - While processing the base clause of SmsManager, it instantiates Natives<SmsManager>
    (to determine its size).

  - While instantiating Natives<SmsManager>, it tries to instantiate the initializer
    of the static constexpr data member.

  - That initializer refers to Impl::NotifyCursorDone, with [Impl = SmsManager].

  - However, at this point SmsManager isn't complete yet (we're still processing
    its base clause!), so it errors out.

I'll further hypothesize that clang is right, and the other compilers are just lenient, deferring the instantiation of the initializer until it's actually needed. I haven't confirmed this, though.

Comment 4

[Botond Ballo [:botond]]

(Note that the size of the class can legitimately depend on the value of static constexpr data member, it just doesn't in this case. For example, if JNINativeMethod had a field "int foo", Natives could have a field of type "std::array<char, methods[0].foo>", and now computing the size of the class requires instantiating the constexpr's initializer. This is what makes me think that clang is right.)

Comment 5

[Botond Ballo [:botond]]

If it's not important for Natives::methods to be constexpr, making it not constexpr and moving its initializer to its definition works around the issue.

Comment 6

[Botond Ballo [:botond]]

After doing some standard reading, I believe clang is correct. I'll explain my reasoning.

To start with, it's clear that processing the base clause "public GeckoSmsManager::Natives<SmsManager>" requires implicit instantiation of GeckoSmsManager::Natives<SmsManager>.

Now we consult [temp.inst] p1:

  [...] The implicit instantiation of a class template specialization causes
  the implicit instantiation of the declarations, but not of the definitions,
  default arguments, or exception-specifications of the class member functions,
  member classes, scoped member enumerations, static data members and member
  templates; [...]

So implicit instantiation of GeckoSmsManager::Natives<SmsManager> causes implicit instantiation of the declaration of GeckoSmsManager::Natives<SmsManager>::methods, but not of its definition.

So the question is, is the initializer considered part of the declaration or part of the definition?

I believe [class.static.data] p3 sheds light on this:

  [...] A static data member of literal type can be declared in the class
  definition with the constexpr specifier; if so, its declaration shall specify
  a brace-or-equal-initializer [...]. The member shall still be defined in a
  namespace scope if it is odr-used in the program and the namespace scope
  definition shall not contain an initializer.

I interpret that as meaning that the initializer, when it appears inside the class definition like this, is considered part of the declaration and not the definition, and so is subject to implicit instantiation.

Comment 7

[Nathan Froyd [:froydnj]]

Thanks for the explanation, Botond.  Looks like we have some fixing to do; I have patches.

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: part 1 - make generated Natives<>::methods[] const instead of constexpr

clang complains that a constexpr definition of methods[] cannot refer to
members of the incomplete Impl template parameter, and rightly so.
Making these const is sufficient for our purposes, and that enables us
to move the declaration outside of the class, where it will be
instantiated lazily (presumably at the point when |Impl| is a complete
class definition).  We also need to declare the length of methods[], as
other parts of the code require knowing the length of methods[] at
compile time.

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: part 2 - more explicit friending between NativeStubImpl and ProxyNativeCall

clang complains about specializations of NativeStubImpl not being able
to see the private constructor of ProxyNativeCall.  While
ProxyNativeCall includes a friend declaration for NativeStubImpl, it's
not obvious *which* NativeStubImpl is being friended, as NativeStubImpl
hasn't been forward declared and exists in a completely separate
namespace.  Forward declaring NativeStubImpl and adjusting the friend
declaration makes everything more correct.

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: part 3 - add MOZ_JNICALL attribute to MakeNativeMethod's argument

clang complains that it's unable to instantiate this template because
the functions being passed as arguments are MOZ_JNICALL, while the
actual parameter to the function has no such attribute.  Adding the
attribute makes everything line up.

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8759654 [details] [diff] [review]
part 1 - make generated Natives<>::methods[] const instead of constexpr

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

LGTM with nits.

::: build/annotationProcessors/CodeGenerator.java
@@ +34,5 @@
>  
>      public CodeGenerator(ClassWithOptions annotatedClass) {
>          this.cls = annotatedClass.wrappedClass;
>          this.clsName = annotatedClass.generatedName;
> +        this.numNativesInits = 0;

Don't need to init to 0.

@@ +343,5 @@
>                  "\n" +
>                  "\n" +
>                  "        mozilla::jni::MakeNativeMethod<" + traits + ">(\n" +
>                  "                mozilla::jni::NativeStub<" + traits + ", Impl>\n" +
>                  "                ::template Wrap<&Impl::" + info.wrapperName + ">)");

Reduce indentation of these lines by 4 spaces.

@@ +569,4 @@
>                  "};\n" +
>                  "\n" +
>                  "template<class Impl>\n" +
> +                "const JNINativeMethod " + clsName + "::Natives<Impl>::methods[] = {" + nativesInits + "\n};" +

Put "};" on separate line with new line at the end, i.e.

> natives.append(
>         ...
>         "const JNINativeMethod " + clsName + "::Natives<Impl>::methods[] = {" + nativesInits + '\n' +
>         "};\n" +
>         "\n");

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a215312a111
part 1 - make generated Natives<>::methods[] const instead of constexpr; r=darchons
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c355864809b
part 2 - more explicit friending between NativeStubImpl and ProxyNativeCall; r=darchons
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cd9cb917c45
part 3 - add MOZ_JNICALL attribute to MakeNativeMethod's argument; r=darchons

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6a215312a111
https://hg.mozilla.org/mozilla-central/rev/9c355864809b
https://hg.mozilla.org/mozilla-central/rev/4cd9cb917c45