RFE: clang support

[su_v]

Follow-up report to
Bug #889172 “Compile failure (clang): "variable length array" errors in box3d.cpp”
<https://bugs.launchpad.net/inkscape/+bug/889172>

Rationale:
- «clang support is a good thing»
<http://thread.gmane.org/gmane.comp.graphics.inkscape.devel/36760/focus=36767>

- OS X is migrating from GCC to CLANG (latest Xcode versions >= 4.2 no longer include Apple's GCC at all). It might be desirable to support building Inkscape using the system compiler on latest OS X versions (compiling Inkscape with custom GCC versions can cause known compatibility issues because the C++ libraries are built with a different compiler version [1]).

Drawbacks:
- Missing OpenMP support (Clang 3.x)

Current status:
Even though most of the reported 'non-POD element type' issues have been fixed in current trunk (and backported to 0.48.x), Inkscape still fails to build with clang:

1) autotools - configure.ac: configure fails with
> checking GNU compiler version...
> configure: error: gcc >= 3.0 is needed to compile inkscape
The check for the compiler version has 'gcc version' hardcoded and fails with clang. [2]

2) unsolved 'variable length array of non-POD element type' errors
- src/2geom/basic-intersection.cpp
- src/2geom/solve-bezier-parametric.cpp

3) blocker: failure to compile 'src/color-profile.cpp'
<http://www.graphicall.org/ftp/ideasman42/inkscape_clang.html>

«I think the problem is that ColorProfileImpl should be defined in the
Inkscape namespace instead of in the global namespace, so that it
matches its declaration in color-profile.h. The code has an using
declaration that pulls ColorProfileImpl into the global namespace and
it looks like GCC then allows defining the type in the scope of the
using declaration, but I doubt whether this is standards conforming.» [3]

Test system:
OS X 10.7.2, MacPorts 2.0.4 (up-to-date)
Inkscape 0.48+devel r11297

Clang versions tested:
- Apple clang version 3.0 (tags/Apple/clang-211.12) (based on LLVM 3.0svn)
- clang version 3.0 (tags/RELEASE_30/final) (MacPorts)
- clang version 3.1 (branches/release_31 155781) (MacPorts)

---
[1] <http://wiki.inkscape.org/wiki/index.php/FAQ#On_Linux.2C_Inkscape_crashes_with_.22invalid_pointer.22_message>
[2] <http://trac.macports.org/ticket/31492>
    <http://fink.cvs.sourceforge.net/fink/dists/10.7/stable/main/finkinfo/graphics/inkscape.info?view=markup#l104>
    <http://www.freebsd.org/cgi/query-pr.cgi?pr=161204>
    <http://clang.debian.net/status.php?version=3.0&key=WRONG_GCC_ASSUMPTION>
[3] <http://thread.gmane.org/gmane.comp.graphics.inkscape.devel/37024/focus=37060>

[su_v (suv-lp)]

Errors in 'src/2geom' (clang 3.1):

  CXX 2geom/basic-intersection.o
../../src/2geom/basic-intersection.cpp:67:26: error: variable length array of
      non-POD element type 'Geom::Point'
    Geom::Point Vtemp[sz][sz];
                         ^
1 error generated.

  CXX 2geom/solve-bezier-parametric.o
../../src/2geom/solve-bezier-parametric.cpp:72:21: error: variable length array of
      non-POD element type 'Geom::Point'
    Geom::Point Left[degree+1], /* New left and right */
                    ^
../../src/2geom/solve-bezier-parametric.cpp:77:34: error: use of undeclared
      identifier 'Right'
    find_parametric_bezier_roots(Right, degree, solutions, depth+1);
                                 ^
../../src/2geom/solve-bezier-parametric.cpp:194:32: error: variable length array
      of non-POD element type 'Geom::Point'
    Geom::Point Vtemp[degree+1][degree+1];
                               ^
3 errors generated.

[su_v (suv-lp)]

Workaround used for 'configure.ac' (not a real fix!):

=== modified file 'configure.ac'
--- configure.ac 2012-04-14 20:28:04 +0000
+++ configure.ac 2012-04-30 17:56:22 +0000
@@ -112,8 +112,8 @@

  # Don't pass CXXFLAGS to the following CXX command as some
  # of them can't be specified along with '-v'.
- cc_version=["`$CXX -v 2>&1 </dev/null |grep 'gcc version' |\
- sed 's/.*gcc version \([-a-z0-9\.]*\).*/\1/'`"]
+ cc_version=["`$CXX -v 2>&1 </dev/null |grep ' version' |\
+ sed 's/.* version \([-a-z0-9\.]*\).*/\1/'`"]

  AC_MSG_RESULT([$cc_version])

[ScislaC (scislac)]

We want this. :)

[su_v (suv-lp)]

Latest Xcode 4.6 (released on 2013-01-28) deprecates LLVM-GCC and GDB:

«Xcode 4.6 is the last major Xcode release which includes the LLVM GCC compiler and the GDB debugger. Please migrate your projects to use the LLVM compiler and LLDB debugger.»
<https://developer.apple.com/library/mac/#documentation/DeveloperTools/Conceptual/WhatsNewXcode/Articles/xcode_4_6.html#//apple_ref/doc/uid/TP40012895-SW1>

[Samuel Chase (samebchase)]

This patch allows building with

Solution I used: http://stackoverflow.com/questions/1403150/how-do-you-dynamically-allocate-a-matrix

Just *one* more file to go! Can someone please suggest for how src/color-profile.cpp may be built successfully with clang++.

[Samuel Chase (samebchase)]

Attached is a file containing the build errors that remain.

[Samuel Chase (samebchase)]

Inkscape now builds with clang++. Attached is the patch for color-profile.cpp. I just followed the advice in the first post and moved ColorProfileImpl into the Inkscape namespace.

I am doing an out of source build. After building, running the generated inkscape binary causes Inkscape to run ( !), but without icons.

I then did:

> make install

Trying to run Inkscape now, results in the program running indefinitely. The GUI just doesn't show up. Is some data file that is loaded at startup causing this issue?

Running gdb seems to stop at inkscape_application_init(..)

Attached is the backtrace.

[Samuel Chase (samebchase)]

Patch for src/color-profile.cpp

[su_v (suv-lp)]

1) the patch 'variable-length-array-error.diff' does not apply - I'm not sure what's wrong with the patch (all hunks fail and are rejected).
Attaching a redone version which does apply.

2) testing build on OS X 10.7.4 with (outdated) Xcode 4.3.2

$ xcodebuild -version
Xcode 4.3.2
Build version 4E2002
$ clang --version
Apple clang version 3.1 (tags/Apple/clang-318.0.58) (based on LLVM 3.1svn)
Target: x86_64-apple-darwin11.4.0
Thread model: posix

Configuration:

        Source code location: ..
        Destination path prefix: /Volumes/cyan/src/inkscape/inkscape-repo/mp-quartz-clang/inst
        Compiler: /usr/bin/clang++
        CPPFLAGS: -DG_DISABLE_DEPRECATED -DGLIBMM_DISABLE_DEPRECATED -DGTK_DISABLE_SINGLE_INCLUDES -DGDKMM_DISABLE_DEPRECATED -DGTK_DISABLE_DEPRECATED -DGTKMM_DISABLE_DEPRECATED -Werror=format-security -DGSEAL_ENABLE -DG_DISABLE_SINGLE_INCLUDES -Wall -Wformat -Wformat-security -W -D_FORTIFY_SOURCE=2 -DWITH_MESH -DLPE_ENABLE_TEST_EFFECTS -I/Volumes/cyan/mp-quartz/with-a-long-long-long-directory-name/include
        CXXFLAGS: -Wpointer-arith -Wcast-align -Wsign-compare -Woverloaded-virtual -Wswitch -Wno-unused-parameter -pipe -O2 -Wno-return-type -Wno-mismatched-tags -Wno-format-security
        CFLAGS: -Wno-pointer-sign -pipe -O2 -Wno-return-type -Wno-mismatched-tags -Wno-format-security
        LDFLAGS: -L/Volumes/cyan/mp-quartz/with-a-long-long-long-directory-name/lib

        Use gnome-vfs: yes
        Use openoffice files: yes
        Use relocation support: no
        Internal Python: skipped
        Internal Perl: skipped
        Enable LittleCms: yes
        Enable DBUS: no
        Enable Poppler-Cairo: yes
        ImageMagick Magick++: yes
        Libwpg: yes
        Libvisio: yes
        Libcdr: yes
        Doing Local Install: no
        GTK+ backend target: quartz

Compilation completes, and the resulting binary launches as expected (does find all resources etc., so that part seems to be a misconfiguration of your build, not related to clang). Further tests with regard to stability and possible regressions yet to be done.

Notes:
- no OpenMP support detected ("unknown"). That's expected AFAIU - with llvm-gcc-4.2, it's at least possible though not so stable (see bug #984836)
- Instructions/help needed with defining correct CFLAGS/CXXFLAGS
- It would be great if someone on Mountain Lion with Xcode 4.6 could test the changes and the resulting builds (personally, I'm not using the latest available versions of OS X and of Xcode (incl. clang), and have no plans to upgrade in the near future)
- PPC support: What about older versions of OS X? AFAIK Xcode 3.1.4 on Mac OS X 10.5.8 Leopard - the last version which runs on PPC arch - does not include clang. Depending on how bug #1122364 is implemented, it is likely to cut off support for PPC Macs completely (if newer GCC versions or clang are required unconditionally).

[su_v (suv-lp)]

Attaching complete diff used with experimental clang build (r12121) on OS X 10.7.4 (with GTK+/Quartz 2.24.15, glib 2.34.3)

Note: this diff includes some other minor changes:
- Fix: support newer automake versions (1.12, 1.13), required with up-to-date MacPorts
- Fix: workaround for preferences dialog with GTK+/Quartz backend
- Enh: add compile date & time back to 'About' dialog
- Enh: add defines for experimental features (gradient mesh, LPETool)

[su_v (suv-lp)]

Attaching build log of first clang build (r12121 - with the minimally required changes applied).

Not sure what to think about these warnings:

  CXXLD inkscape
  CXXLD inkview
clang: warning: argument unused during compilation: '-pthread'
clang: warning: argument unused during compilation: '-pthread'

[jazzynico (jazzynico)]

Patch from comment #10 tested on successfully on Windows XP, Inkscape trunk revision 12119. Inkscape still compiles as expected, and no regression found so far.

[su_v (suv-lp)]

> Patch from comment #10 tested on successfully on Windows XP

Oops - seems that I made quite a mess with what I attached earlier: that patch from comment #10 includes additional changes which I didn't plan to propose for trunk (at least not as part of this report about clang support)…

Attaching a minimal version (changes unrelated to clang support removed) for trunk r12121:
- configure.ac (workaround to not misread clang as GCC <= 3.0
- Samuel's patches to 'src/2geom' and 'src/color-profile.cpp'

Note: the compiler detection in configure.ac needs a proper fix (the changes as described in comment #2 are just a workaround, not a real fix).

[jazzynico (jazzynico)]

> that patch from comment #10 includes additional changes which I didn't plan to propose for trunk

Interesting changes that would be nice to have in the trunk ;)

[Samuel Chase (samebchase)]

Hi ~suv and JazzyNico,

Were you able to get Inkscape to run properly?

The the C/C++ libraries on my computer have been built with GCC.

Is code that is compiled with clang/clang++, but linking to libraries compiled with GCC expected to work.

I'm not able to get Inkscape build with clang to run on ArchLinux. What could be the possible reason for this?

Stepping with gdb led me to discover that a memset() call executes indefinitely in the function XmlSource::setFile() in the file repr-io.cpp when an (internal) SVG is read at startup.

[Samuel Chase (samebchase)]

Also, to fix the variable length array issue, I dynamically allocated a two dimensional array in a few places, but I have not released the memory after use. This is a memory leak, right?

[Samuel Chase (samebchase)]

> Compilation completes, and the resulting binary launches as expected
> (does find all resources etc., so that part seems to be a misconfiguration of your build, not related to clang).

Okay. I'll do a fresh build and see if I can get it working on Linux.

[jazzynico (jazzynico)]

Samuel> Were you able to get Inkscape to run properly?

I just did some basic tests, nothing to do with extensive regression tests. All I can say is that it compiles on Windows, launches as expected and runs with no obvious regression. Note that I didn't use clang to compile, but the usual TDM-GCC compiler.
But if you know there are memory leaks, it's worth fixing it before committing the patch.

[su_v (suv-lp)]

@Samuel - I'm not able to really help with questions about clang, neither on ArchLinux (or other Linux distros) nor on OS X, sorry. Same with regard to questions about memory leaks (but if your patch creates some, please fix it ;-) ).

With regard to the first patch for files in 'src/2geom': maybe you can address the compiler warnings, too? They occur with old and newer GCC versions, as well as with Clang:

../../src/2geom/basic-intersection.cpp:70:23: warning: comparison of integers of different signs: 'int' and 'const unsigned int' [-Wsign-compare]
    for (int i = 0; i < sz; ++i) {
                    ~ ^ ~~

../../src/2geom/recursive-bezier-intersection.cpp:87:23: warning: comparison of integers of different signs: 'int' and 'const unsigned int' [-Wsign-compare]
    for (int i = 0; i < sz; ++i) {
                    ~ ^ ~~

../../src/2geom/solve-bezier-parametric.cpp:201:23: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
    for (int i = 0; i < degree + 1; ++i) {
                    ~ ^ ~~~~~~~

[Samuel Chase (samebchase)]

This should fix the warnings and the memory leaks in two locations.

However, I am not sure how I should go about freeing the memory in find_parametric_bezier_roots() in solve-bezier-parametric.cpp. It is a recursive function that takes the allocated arrays as parameters.

Can someone please take a look at this function.

[su_v (suv-lp)]

@Samuel - I don't know why, but your patch again fails to apply cleanly here (yes, I noticed that it is not against unmodified trunk, and patches an already patched version of the 'src/2geom' files: but still, 2 hunks do not apply). Do I have to manually fix this locally, or is there some option I could use with the 'patch' command to force it to apply your diffs?

[Samuel Chase (samebchase)]

Internet access issues. :-(

Would making a diff against the latest trunk work? i.e. All the
changes in one big diff?

I'll make that diff and upload asap.

On 2/14/13, ~suv <email address hidden> wrote:
> @Samuel - I don't know why, but your patch again fails to apply cleanly
> here (yes, I noticed that it is not against unmodified trunk, and
> patches an already patched version of the 'src/2geom' files: but still,
> 2 hunks do not apply). Do I have to manually fix this locally, or is
> there some option I could use with the 'patch' command to force it to
> apply your diffs?
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/992124
>
> Title:
> RFE: clang support
>
> Status in Inkscape: A Vector Drawing Tool:
> In Progress
>
> Bug description:
> Follow-up report to
> Bug #889172 “Compile failure (clang): "variable length array" errors in
> box3d.cpp”
> <https://bugs.launchpad.net/inkscape/+bug/889172>
>
> Rationale:
> - «clang support is a good thing»
>
> <http://thread.gmane.org/gmane.comp.graphics.inkscape.devel/36760/focus=36767>
>
> - OS X is migrating from GCC to CLANG (latest Xcode versions >= 4.2 no
> longer include Apple's GCC at all). It might be desirable to support
> building Inkscape using the system compiler on latest OS X versions
> (compiling Inkscape with custom GCC versions can cause known
> compatibility issues because the C++ libraries are built with a
> different compiler version [1]).
>
> Drawbacks:
> - Missing OpenMP support (Clang 3.x)
>
> Current status:
> Even though most of the reported 'non-POD element type' issues have been
> fixed in current trunk (and backported to 0.48.x), Inkscape still fails to
> build with clang:
>
> 1) autotools - configure.ac: configure fails with
> > checking GNU compiler version...
> > configure: error: gcc >= 3.0 is needed to compile inkscape
> The check for the compiler version has 'gcc version' hardcoded and fails
> with clang. [2]
>
> 2) unsolved 'variable length array of non-POD element type' errors
> - src/2geom/basic-intersection.cpp
> - src/2geom/solve-bezier-parametric.cpp
>
> 3) blocker: failure to compile 'src/color-profile.cpp'
> <http://www.graphicall.org/ftp/ideasman42/inkscape_clang.html>
>
> «I think the problem is that ColorProfileImpl should be defined in the
> Inkscape namespace instead of in the global namespace, so that it
> matches its declaration in color-profile.h. The code has an using
> declaration that pulls ColorProfileImpl into the global namespace and
> it looks like GCC then allows defining the type in the scope of the
> using declaration, but I doubt whether this is standards conforming.» [3]
>
> Test system:
> OS X 10.7.2, MacPorts 2.0.4 (up-to-date)
> Inkscape 0.48+devel r11297
>
> Clang versions tested:
> - Apple clang version 3.0 (tags/Apple/clang-211.12) (based on LLVM
> 3.0svn)
> - clang version 3.0 (tags/RELEASE_30/final) (MacPorts)
> - clang version 3.1 (branches/release_31 155781) (MacPorts)
>
> ---
> [1]
> <http://wiki.inkscape.org/wiki/index.php/FAQ#On_Linux.2C_Inkscape_crashes_with_.22...

[su_v (suv-lp)]

> Would making a diff against the latest trunk work?

What would it change? I don't need other modifications you might have in your local branch: the question to me is whether your patches of the files in 'src/2geom' had been against a modified base already, or whether you have some other whitespace/formatting changes in your branch which don't work nicely with 'bzr diff' and 'patch' …

Attaching the diff against r12122 for the modified files in 'src/2geom' I'm currently using and which applies cleanly to trunk:
$ bzr diff src/2geom > uint-comparison-warn-and-memleaks-r12122.diff

Please verify!

[Samuel Chase (samebchase)]

Hello ~suv,

Yes, that patch looks alright.

I manually deleted some lines from the patch at the start which were modifications to autgen.sh and configure.ac to work with my new version of automake.

The modified patch cleanly applies to trunk:r12114.

[Samuel Chase (samebchase)]

Should a 'clang-build' branch be created, and new commits pulled from trunk, so that it will be easy to make sure whether trunk builds?

[su_v (suv-lp)]

Samuel Chase wrote:
> Should a 'clang-build' branch be created, and new commits pulled
> from trunk, so that it will be easy to make sure whether trunk builds?

Feel free to do so if you think it helps to get more clang build tests done on various platforms (and with different versions/flavors of clang), along with tests with various GCC versions (to test for possible regressions introduced for other compilers).

Currently, I build trunk with clang [1] several times a week (using local branches of trunk with the changes we have so far applied), and can report regressions which cause it to fail to compile (runtime tests are only minimal ATM). This works ok for me as long as there are as few changes as currently required. I also have the same patches applied to my regular branches which compile with llvm-gcc-4.2 (to test for unintentional side-effects).
OTOH having a branch pushed to launchpad, which is kept up-to-date with regular merges from trunk, would make it easy to create up-to-date diffs anytime…

Right now, I'm more worried about getting 'configure.ac' ready for clang, proper recommendations for CFLAGS/CXXFLAGS [2] (which probably also could be addressed in configure.ac), and about the lack of OpenMP support in clang (even if OpenMP support with llvm-gcc.4.2 is not very stable (bug #984836), the difference is still quite noticeable, even with only a few blurs in a drawing).

-----
[1] a somewhat dated version from Apple (see comment #9 for details)
[2] without additional flags (e.g. --Wno-mismatched-tags), I got a huge build log file (3.4MB):
Chillida:mp-quartz-clang su_v$ grep 'Wmismatched-tags' ../../_log/2013-02-15-mp-quartz-clang-12129.txt | wc -l
    9456

[su_v (suv-lp)]

Attaching list of current warnings with rev 12175 (clean build).

[Alex Valavanis (valavanisalex)]

I've applied the proposed fixes, and hopefully sorted out the remaining array allocation/deletion issues in lp:inkscape r12202. I now successfully build trunk with clang on Ubuntu 12.10 amd64 after disabling the configure.ac check for gcc 3.0. Inkscape seems to run fine.

Regarding the issues raised previously:

* Lack of OpenMP support: yes, this is problematic, but I don't think we can really do anything other than wait for clang to introduce it. Inkscape will still compile and run without it, so I guess we can output a warning message from the configure script to make the issue clear to the user.

* Large build log: this is probably a good thing... if clang's fussy about the quality of our code, it gives us a good "to do" list!

* Infinite loop on ArchLinux: Are we sure this is a clang-specific issue? Does it work properly with gcc?

[su_v (suv-lp)]

> Large build log: this is probably a good thing...

Do you happen to know more about the '-Wno-mismatched-tags' flag? AFAIU '-Wmismatched-tags' gets enabled with '-Wall', and creates so huge logs that it's hard to detect e.g. relevant (new) warnings… - could that warning possibly be disabled via configure.ac, without harm?

[su_v (suv-lp)]

Regarding the issues raised previously (cont.):

* Darwin (OS X): clang: warning: argument unused during compilation: '-pthread'
According to e.g. <http://llvm.org/bugs/show_bug.cgi?id=7798> (and other search results via Google) '-pthread' is not required on Darwin (but currently set for all platforms (and compilers) via configure.ac).
Some projects seem to suppress it with '-Qunused-arguments' in CFLAGS/CXXFLAGS for clang, others remove '-pthread' for platform 'Darwin'.

[Alex Valavanis (valavanisalex)]

A lot of the new [-Wmismatched-tags] warnings in the build log come from dodgy forward declarations where something that is really a class is declared as a struct or vice-versa. Strictly, it's not great to allow this, because class and struct imply different levels of default privacy (class="private by default"; struct="public by default"), so we shouldn't really get rid of the warning.

The good news is that these are easy fixes, and that a lot of it is duplicated. i.e., simply changing "class" for "struct" in one line of code in a widely-used header could fix thousands of these warnings.

[Alex Valavanis (valavanisalex)]

Dropped the ancient gcc version check and started fixing a load of the broken forward declarations in lp:inkscape r12203.

[Alex Valavanis (valavanisalex)]

As of r12205, we're down to "only" 4279 lines in the build log by my reckoning.

[Alex Valavanis (valavanisalex)]

I get a lot of messages:

clang: warning: argument unused during compilation: '-fopenmp'

This appears to be the result of ImageMagick++ specifying -fopenmp as a cflag:

$ pkg-config ImageMagick++ --cflags
-fopenmp -I/usr/include/ImageMagick

...so I'm not convinced we can do much to fix the issue other than wait for clang to introduce openmp support

[Alex Valavanis (valavanisalex)]

As noted above, we can silence the warnings about -fopenmp (or other flags) not being used, with:

CPPFLAGS+="-Qunused-arguments"

Not sure if this is really desirable or not.

[Alex Valavanis (valavanisalex)]

I've cleaned up the remaining forward declaration tag issues in lp:inkscape r12208. There are still a couple of warnings caused by dodgy declarations in the gtkmm headers. They are, however, harmless and we can't do anything about them except wait for it to be fixed in gtkmm.

Latest build log attached.

[su_v (suv-lp)]

Alex Valavanis wrote on 2013-03-14:
> Dropped the ancient gcc version check and started fixing a
> load of the broken forward declarations in lp:inkscape r12203.

Now compiler version check fails later on in configure:

(…)
checking for CAIRO_USER_FONTS... yes
../configure: line 22142: test: -gt: unary operator expected
checking for INKSCAPE... yes
(…)

Looks as if this test in 'configure.ac' fails (>= r12203):

| dnl ******************************
| dnl Unconditional dependencies
| dnl ******************************
|
| dnl sigc++-2.0 >= 2.0.12: using "visit_each" not available in 2.0.10
| if test $cc_vers_major -gt 3; then
| min_sigc_version=2.0.12
| else
| min_sigc_version=2.0.11
| fi

[su_v (suv-lp)]

Alex Valavanis wrote:
> I've cleaned up the remaining forward declaration tag issues
> in lp:inkscape r12208.

r12206 breaks compiling GTK3 builds, tested on OS X 10.7 with
- GTK+/X11 3.4.4 (gtkmm 3.4.0, poppler 0.18.4)
- GTK+/Quartz 3.6.2 (gtkmm 3.6.0, poppler 0.22.1)
using Apple's llvm-gcc-4.2 and Apple's clang 3.1 (Xcode 4.3.2)

Error messages from 'src/extension/internal/pdfinput/pdf-input.cpp':
 error: invalid use of incomplete type ‘struct Gtk::CheckButton’
 error: no matching function for call to ‘Gtk::HBox::pack_start(Gtk::CheckButton&, Gtk::PackOptions, int)’

Attached logfile is from build on OS X 10.7 using
llvm-gcc-4.2, GTK+/Quartz 3.6.2, gtkmm 3.6.0, poppler 0.22.1

[su_v (suv-lp)]

~suv wrote:
> r12206 breaks compiling GTK3 builds, tested on OS X 10.7 (…)

Confirmed fixed in r12209 (all builds succeed).

[Alex Valavanis (valavanisalex)]

Build-log attached for:

CPPFLAGS+="-Qunused-arguments" CC="clang" CXX="clang++" ../configure

I haven't inspected it in detail, but it doesn't look too scary.

[su_v (suv-lp)]

The changes in r12112 to suppress compiler warnings use '-Wno-unused-but-set-variable' which is a relative new option only available in recent versions of GCC (introduced with FSF GCC 4.6). Older versions of GCC (apparently 4.2 and 4.3) don't ignore the unknown flag, but fail with an error [1]. The changes of r12112 break building for example on OS X with Apple's llvm-gcc-4.2 compiler:

  CC libgdl/libgdl_libgdl_a-gdl-i18n.o
  CC libgdl/libgdl_libgdl_a-gdl-dock-object.o
  CC libgdl/libgdl_libgdl_a-gdl-dock-master.o
  CC libgdl/libgdl_libgdl_a-gdl-dock.o
cc1: error: unrecognized command line option "-Wno-unused-but-set-variable"
cc1: error: unrecognized command line option "-Wno-unused-but-set-variable"
cc1: error: unrecognized command line option "-Wno-unused-but-set-variable"
cc1: error: unrecognized command line option "-Wno-unused-but-set-variable"
make[3]: *** [libgdl/libgdl_libgdl_a-gdl-dock.o] Error 1
make[3]: *** Waiting for unfinished jobs....
make[3]: *** [libgdl/libgdl_libgdl_a-gdl-dock-master.o] Error 1
make[3]: *** [libgdl/libgdl_libgdl_a-gdl-i18n.o] Error 1
make[3]: *** [libgdl/libgdl_libgdl_a-gdl-dock-object.o] Error 1
make[2]: *** [all] Error 2
make[1]: *** [all-recursive] Error 1
make: *** [all] Error 2

Could this flag be added conditionally, rather than be the reason to completely remove support for older GCC versions altogether at this point?

-----
[1] see e.g. <http://article.gmane.org/gmane.network.freerdp.devel/1354>

[Alex Valavanis (valavanisalex)]

Ah, sorry about that. Will look into a conditional way of setting this.

[Alex Valavanis (valavanisalex)]

Hopefully the implementation in r12216 should work

[su_v (suv-lp)]

> Hopefully the implementation in r12216 should work

Nope - same failure. AFAICT there seems to be no test done for that specific option '-Wno-unused-but-set-variable'. The tested flags as output by 'configure' are still the same as before:

(…)
checking compiler support for -Werror=format-security... yes
checking compiler support for -Wno-pointer-sign... yes
(…)

and further down in configure.ac (line 904), one additional (global) compiler flag is tested: '-Wno-unused-parameter' (AFAICT this test creates no output from configure, but is noted in 'build/config.log').

[su_v (suv-lp)]

Fix in r12219 confirmed:
builds with Apple GCC 4.2.1 (Mac OS X 10.5.8), Apple llvm-gcc-4.2 and Apple clang 3.1 (OS X 10.7) all succeed

[Alex Valavanis (valavanisalex)]

I wonder if we have a clear goal with this request. If it's a case of just being able to compile with clang, then I think the report can be closed now. Similarly, if it's a case of being able to compile "cleanly", I think we're about as clean as with GCC.

As far as I can tell, the only remaining issue is that clang doesn't offer OpenMP support... and there's nothing we can do about that other than wait for upstream; Inkscape will compile and run, but with some performance loss.

Can we mark this as "Fix Committed"?

[Samuel Chase (samebchase)]

I guess so. Will we have to open a bug for the lack of OpenMP support in
Inkscape, or is that considered a clang bug or a bug in OpenMP itself?

On Thu, Apr 11, 2013 at 3:38 PM, Alex Valavanis
<email address hidden>wrote:

> I wonder if we have a clear goal with this request. If it's a case of
> just being able to compile with clang, then I think the report can be
> closed now. Similarly, if it's a case of being able to compile
> "cleanly", I think we're about as clean as with GCC.
>
> As far as I can tell, the only remaining issue is that clang doesn't
> offer OpenMP support... and there's nothing we can do about that other
> than wait for upstream; Inkscape will compile and run, but with some
> performance loss.
>
> Can we mark this as "Fix Committed"?
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/992124
>
> Title:
> RFE: clang support
>
> Status in Inkscape: A Vector Drawing Tool:
> In Progress
>
> Bug description:
> Follow-up report to
> Bug #889172 “Compile failure (clang): "variable length array" errors in
> box3d.cpp”
> <https://bugs.launchpad.net/inkscape/+bug/889172>
>
> Rationale:
> - «clang support is a good thing»
> <
> http://thread.gmane.org/gmane.comp.graphics.inkscape.devel/36760/focus=36767
> >
>
> - OS X is migrating from GCC to CLANG (latest Xcode versions >= 4.2 no
> longer include Apple's GCC at all). It might be desirable to support
> building Inkscape using the system compiler on latest OS X versions
> (compiling Inkscape with custom GCC versions can cause known
> compatibility issues because the C++ libraries are built with a
> different compiler version [1]).
>
> Drawbacks:
> - Missing OpenMP support (Clang 3.x)
>
> Current status:
> Even though most of the reported 'non-POD element type' issues have been
> fixed in current trunk (and backported to 0.48.x), Inkscape still fails to
> build with clang:
>
> 1) autotools - configure.ac: configure fails with
> > checking GNU compiler version...
> > configure: error: gcc >= 3.0 is needed to compile inkscape
> The check for the compiler version has 'gcc version' hardcoded and fails
> with clang. [2]
>
> 2) unsolved 'variable length array of non-POD element type' errors
> - src/2geom/basic-intersection.cpp
> - src/2geom/solve-bezier-parametric.cpp
>
> 3) blocker: failure to compile 'src/color-profile.cpp'
> <http://www.graphicall.org/ftp/ideasman42/inkscape_clang.html>
>
> «I think the problem is that ColorProfileImpl should be defined in the
> Inkscape namespace instead of in the global namespace, so that it
> matches its declaration in color-profile.h. The code has an using
> declaration that pulls ColorProfileImpl into the global namespace and
> it looks like GCC then allows defining the type in the scope of the
> using declaration, but I doubt whether this is standards conforming.» [3]
>
> Test system:
> OS X 10.7.2, MacPorts 2.0.4 (up-to-date)
> Inkscape 0.48+devel r11297
>
> Clang versions tested:
> - Apple clang version 3.0 (tags/Apple/clang-211.12) (based on LLVM
> 3.0svn)
> - clang version 3.0 (tags/RELE...

[su_v (suv-lp)]

> Can we mark this as "Fix Committed"?

Agreed.

> Will we have to open a bug for the lack of OpenMP support
> in Inkscape, or is that considered a clang bug or a bug in
> OpenMP itself?

In my understanding missing OpenMP support is a clang issue.

[Niederstrasser (niederstrasser)]

I have backported multiple patches that were needed to compile the 0.48.x branch with clang. They are now attached to bug #1194129. With them and bug #1193025, inkscape-0.48.x now compiles and runs when built with clang on OS X 10.7.

[Samuel Chase (samebchase)]

It looks like people are working on adding OpenMP support to Clang:
http://clang-omp.github.io/

Haven't tried this, yet, but I wonder how long it will be before we
can have a fully-featured Clang build of Inkscape.

Will that mean we can then start using C++11 features as all platforms
will have modern C++ compilers?

[su_v (suv-lp)]

On 2013-08-27 21:29 +0200, Samuel Chase wrote:
> (…) but I wonder how long it will be before we can have a
> fully-featured Clang build of Inkscape.
>
> Will that mean we can then start using C++11 features as all platforms
> will have modern C++ compilers?

Samuel, please take this discussion to the developers' mailing list; see e.g. this related recent thread:
<http://thread.gmane.org/gmane.comp.graphics.inkscape.devel/40229/focus=40239>

[Samuel Chase (samebchase)]

Hello ~suv,

Cool, thanks.

Samuel