After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 778046 - Webkit2: does not build with clang
Webkit2: does not build with clang
Status: RESOLVED FIXED
Product: geary
Classification: Other
Component: build
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Geary Maintainers
Geary Maintainers
wk2-fallout
Depends on:
Blocks:
 
 
Reported: 2017-02-01 21:56 UTC by Gautier Pelloux-Prayer
Modified: 2017-02-06 06:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
clang logs of cmake and make commands (3.03 MB, text/plain)
2017-02-01 21:56 UTC, Gautier Pelloux-Prayer
  Details
patch proposal v1 (2.83 KB, patch)
2017-02-03 14:21 UTC, Gautier Pelloux-Prayer
committed Details | Review

Description Gautier Pelloux-Prayer 2017-02-01 21:56:58 UTC
Created attachment 344746 [details]
clang logs of cmake and make commands

Compiling with gcc is fine, but it fails with clang due to many errors like the following:

> geary/src/engine/util/util-js.vala:88:4: error: non-void function 'geary_js_to_object' should return a value [-Wreturn-type]
                        return;

Full logs attached.

It might be related to this: http://clang.debian.net/status.php?version=3.5.0&key=FUNCTION_RETURNS_VALUE

Clang version:

clang version 3.8.1-17 (tags/RELEASE_381/final)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/4.8
Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/4.8.4
Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/4.9
Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/4.9.2
Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/5.4.1
Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/6.3.0
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.8
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.8.4
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.9
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.9.2
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/5.4.1
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/6.3.0
Selected GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/6.3.0
Candidate multilib: .;@m64
Candidate multilib: 32;@m32
Candidate multilib: x32;@mx32
Selected multilib: .;@m64

Gcc version:

Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/6/lto-wrapper
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Debian 6.3.0-5' --with-bugurl=file:///usr/share/doc/gcc-6/README.Bugs --enable-languages=c,ada,c++,java,go,d,fortran,objc,obj-c++ --prefix=/usr --program-suffix=-6 --program-prefix=x86_64-linux-gnu- --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-vtable-verify --enable-libmpx --enable-plugin --enable-default-pie --with-system-zlib --disable-browser-plugin --enable-java-awt=gtk --enable-gtk-cairo --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-6-amd64/jre --enable-java-home --with-jvm-root-dir=/usr/lib/jvm/java-1.5.0-gcj-6-amd64 --with-jvm-jar-dir=/usr/lib/jvm-exports/java-1.5.0-gcj-6-amd64 --with-arch-directory=amd64 --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --with-target-system-zlib --enable-objc-gc=auto --enable-multiarch --with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32 --enable-multilib --with-tune=generic --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
gcc version 6.3.0 20170124 (Debian 6.3.0-5)
Comment 1 Michael Gratton 2017-02-02 05:32:06 UTC
This looks like a vala codegen error - the vala code is correct, but the generated C is simply using a void return.

Which version vala are you using? Bug 744639 looks related, but it was fixed ages ago and it deals with codegen in a different context (lambdas), so there may be another case to be addressed there.
Comment 2 Gautier Pelloux-Prayer 2017-02-02 10:36:12 UTC
I'm using Vala 0.34.4 - however that's strange that it breaks only on wip/728002-webkit2 branch and not on master (before the merge!).
Do you know what could have changed so much?
Comment 3 Michael Gratton 2017-02-03 01:10:54 UTC
Well, src/engine/util/util-js.vala is all-new for the WK2 port, so I guess there's some aspect of the new code there that triggers the issue. It might be the return type (a vala SimpleType, which despite being object-like is pass-by-copy rather than pass-by-reference), or maybe the namespace collision that's happening there, necessitating the use of the global:: modifier for JSC objects.

Or it could be something else. Do you have a complete list of the problematic functions/methods?

I don't have clang installed, so if you have a moment some workarounds to try might include:

 - Marking the return type as nullable with a '?' might nudge the codegen in the right direction
 - Rename the util-js.vala namespace to Geary.JSUtil, remove all uses use of global:: in that file, and update call sites

If neither of those things work, we'd need to modify the methods to either return null when an error occurs instead of throwing an exception, or (preferably) keeping the exception, making them into void functions and make the return value an out param.
Comment 4 Gautier Pelloux-Prayer 2017-02-03 14:21:43 UTC
Created attachment 344865 [details] [review]
patch proposal v1

Thanks for the hint. Adding nullable ? to return type solves the problem, patch attached.
Comment 5 Michael Gratton 2017-02-06 06:04:27 UTC
Review of attachment 344865 [details] [review]:

Committed as f5f1220, thanks. I added some comments so people know what's going on with the nullable types in the future.

I filed Bug 778224 against vala for the underlying issue, we should remove the workaround when it gets fixed and can depend on the version it is released as.