Bug 207253 - cad/openvsp: Fix in 11-CURRENT
Summary: cad/openvsp: Fix in 11-CURRENT
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: Raphael Kubo da Costa
URL:
Keywords: easy, patch, patch-ready
Depends on:
Blocks:
 
Reported: 2016-02-16 19:03 UTC by Fernando Apesteguía
Modified: 2016-02-27 10:30 UTC (History)
5 users (show)

See Also:
koobs: merge-quarterly?


Attachments
Patch to the ports tree (480 bytes, patch)
2016-02-16 19:03 UTC, Fernando Apesteguía
fernape: maintainer-approval+
Details | Diff
poudriere log on 9.3-RELEASE-amd64 (225.94 KB, text/x-log)
2016-02-16 19:04 UTC, Fernando Apesteguía
no flags Details
poudriere log on 10.2-RELEASE-amd64 (418.40 KB, text/x-log)
2016-02-16 19:05 UTC, Fernando Apesteguía
no flags Details
port test output for 11-CURRENT (187.44 KB, text/x-log)
2016-02-16 19:06 UTC, Fernando Apesteguía
no flags Details
Enclose array in a namespace (4.33 KB, patch)
2016-02-25 10:43 UTC, Raphael Kubo da Costa
rakuco: maintainer-approval+
Details | Diff
poudriere log on 9.3-RELEASE-amd64 (204.42 KB, text/x-log)
2016-02-25 19:04 UTC, Fernando Apesteguía
no flags Details
poudriere log on 10.2-RELEASE-amd64 (217.02 KB, text/x-log)
2016-02-25 19:05 UTC, Fernando Apesteguía
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Fernando Apesteguía freebsd_committer freebsd_triage 2016-02-16 19:03:45 UTC
Created attachment 167088 [details]
Patch to the ports tree

Long overdue task...

Fix compilation in 11-CURRENT by specifying gcc as the default compiler for the port.

Attached poudriere logs for {9.3,10.2}amd64 and port test output for 11.0-CURRENT-i386-20160206-r295345.

The application runs fine with this change.

Nevertheless, in 9.3 and 10.2 the port compiles fine with clang from base system so I'm inclined to think that this is some kind of regression in clang 3.7.1 in CURRENT. The port broke around 11/7/2015.
Comment 1 Fernando Apesteguía freebsd_committer freebsd_triage 2016-02-16 19:04:19 UTC
Created attachment 167089 [details]
poudriere log on 9.3-RELEASE-amd64
Comment 2 Fernando Apesteguía freebsd_committer freebsd_triage 2016-02-16 19:05:18 UTC
Created attachment 167090 [details]
poudriere log on 10.2-RELEASE-amd64
Comment 3 Fernando Apesteguía freebsd_committer freebsd_triage 2016-02-16 19:06:06 UTC
Created attachment 167091 [details]
port test output for 11-CURRENT
Comment 4 Kubilay Kocak freebsd_committer freebsd_triage 2016-02-17 04:40:43 UTC
Thanks Fernando

For this and future issues, please declare "maintainer approval" on attachments for ports you are MAINTAINER by setting the attachment flag value in Attachment -> Details -> maintainer-approval [+]
Comment 5 Kubilay Kocak freebsd_committer freebsd_triage 2016-02-17 04:41:26 UTC
Ignore previous comment, I see that is has been set.

Thanks!
Comment 6 Raphael Kubo da Costa freebsd_committer freebsd_triage 2016-02-17 12:00:42 UTC
Looking at the build failure, it seems to be caused by src/util_code/array.h declaring a class called array: https://github.com/OpenVSP/OpenVSP/blob/v2_master/src/util_code/array.h, which clashes with std::array when libc++ is used because a lot of files do 'using namespace std'.

It's possible to work around the issue by changing the name of the class or wrapping it around another namespace

However, I don't think this clash should happen, as -std=c++11 (or 0x etc) is not being passed to the compiler. The __tuple header seems to be unconditionally declaring a 'struct array' here: https://github.com/freebsd/freebsd/blob/ea5248c/contrib/libc%2B%2B/include/__tuple#L116

Dimitry, is this working as expected from libc++'s perspective?
Comment 7 Raphael Kubo da Costa freebsd_committer freebsd_triage 2016-02-24 13:29:05 UTC
+freebsd-toolchain. Can someone take a look at comment #6 and check if libc++ is working as intended?
Comment 8 Dimitry Andric freebsd_committer freebsd_triage 2016-02-24 15:14:12 UTC
I think the program should not try to use both "using namespace std", and then use a reserved C++11 name.  It should either drop the "using namespace std", or rename its own array to e.g. openvsp_array or even better, ovenvsp::array.

In general, try to avoid "using namespace std".
Comment 9 Raphael Kubo da Costa freebsd_committer freebsd_triage 2016-02-24 15:19:34 UTC
I agree, but this is legacy software (version 3 doesn't even have its own array implementation anymore as far as I can see) that won't be changed upstream.

My question is if libc++ is right in declaring std::array when not in C++11 mode or if this clash shouldn't happen at all, even if what the program does is dangerous.
Comment 10 Fernando Apesteguía freebsd_committer freebsd_triage 2016-02-24 16:25:24 UTC
(In reply to Raphael Kubo da Costa from comment #9)

I think it is not doing the right thing. Have a look at this[1]. std::array is a feature introduced in C++11 and the software is not using C++11 features.

[1] http://www.cplusplus.com/reference/array/array/
Comment 11 Dimitry Andric freebsd_committer freebsd_triage 2016-02-24 19:01:48 UTC
(In reply to fernando.apesteguia from comment #10)
> (In reply to Raphael Kubo da Costa from comment #9)
> 
> I think it is not doing the right thing. Have a look at this[1]. std::array
> is a feature introduced in C++11 and the software is not using C++11
> features.
> 
> [1] http://www.cplusplus.com/reference/array/array/

Well, try to report this to libc++ upstream, and see what they say.  I don't much feel like hacking our local copy with some change that will conflict with whatever upstream is doing, let alone that we cannot do anything about the existing copies of libc++ out there.

If openvsp is legacy software, it should be no problem to put a small patch in which fixes this.
Comment 12 Fernando Apesteguía freebsd_committer freebsd_triage 2016-02-24 23:01:47 UTC
(In reply to Dimitry Andric from comment #11)

What strikes me is that the very same software works on FreeBSD 9.x and 10.x without issues (http://portsmon.freebsd.org/portoverview.py?category=cad&portname=openvsp). So something must have changed in libc++. I'm afraid we can break other software as stupid as they can be using names. I think it's worth having a look at what it changed around July 2015 that was when the port started failing.

I'm working on the port of OpenVSP 3.0 but it requires the addition of other ports first and it will take a while. In the meantime I would rather compile OpenVSP with gcc as the patch purposes instead of working on a new patch for the code.
Comment 13 Raphael Kubo da Costa freebsd_committer freebsd_triage 2016-02-25 10:43:26 UTC
Created attachment 167391 [details]
Enclose array in a namespace

(In reply to Dimitry Andric from comment #11) 
> Well, try to report this to libc++ upstream, and see what they say.  I don't
> much feel like hacking our local copy with some change that will conflict
> with whatever upstream is doing, let alone that we cannot do anything about
> the existing copies of libc++ out there.

OK. I was hoping you'd be willing to do that as it happened a few other times in the past.

(In reply to fernando.apesteguia from comment #12)
> What strikes me is that the very same software works on FreeBSD 9.x and 10.x
> without issues
> (http://portsmon.freebsd.org/portoverview.py?category=cad&portname=openvsp).
> So something must have changed in libc++. I'm afraid we can break other
> software as stupid as they can be using names. I think it's worth having a
> look at what it changed around July 2015 that was when the port started
> failing.

What changed is that libc++ and the rest of clang/LLVM were updated to more recent versions during that period, and these new versions introduced the changes that are breaking OpenVSP.

> I'm working on the port of OpenVSP 3.0 but it requires the addition of other
> ports first and it will take a while. In the meantime I would rather compile
> OpenVSP with gcc as the patch purposes instead of working on a new patch for
> the code.

Please take a look at this patch I'm attaching. It builds fine on HEAD for me, and doesn't force the use of GCC.
Comment 14 Fernando Apesteguía freebsd_committer freebsd_triage 2016-02-25 19:04:50 UTC
Created attachment 167408 [details]
poudriere log on 9.3-RELEASE-amd64
Comment 15 Fernando Apesteguía freebsd_committer freebsd_triage 2016-02-25 19:05:17 UTC
Created attachment 167409 [details]
poudriere log on 10.2-RELEASE-amd64
Comment 16 Fernando Apesteguía freebsd_committer freebsd_triage 2016-02-25 19:06:52 UTC
Comment on attachment 167391 [details]
Enclose array in a namespace

I added two new poudriere logs for {9.3,10.2}amd64. I can also confirm the application runs fine on 10.2
Comment 17 Raphael Kubo da Costa freebsd_committer freebsd_triage 2016-02-25 19:07:21 UTC
Shall we proceed with this version then?
Comment 18 Fernando Apesteguía freebsd_committer freebsd_triage 2016-02-25 19:09:39 UTC
(In reply to Raphael Kubo da Costa from comment #17)

Yes, please. And thanks to come up with the patch.
Comment 19 commit-hook freebsd_committer freebsd_triage 2016-02-25 22:22:00 UTC
A commit references this bug:

Author: rakuco
Date: Thu Feb 25 22:21:38 UTC 2016
New revision: 409563
URL: https://svnweb.freebsd.org/changeset/ports/409563

Log:
  Add patches to fix the build on FreeBSD 11 with libc++.

  OpenVSP does something like this:
    using namespace std;
    class array {
      ...
    };

  Which causes the build to fail with HEAD's libc++. Even though the port does
  not use -std=c++11, libc++ still declares an array class that conflicts with
  the one OpenVSP has.

  Enclose OpenVSP's array declaration in a namespace to avoid these conflicts.

  PR:		207253
  Approved by:	fernando.apesteguia@gmail.com (maintainer)

Changes:
  head/cad/openvsp/files/patch-src_util__code_array.h
  head/cad/openvsp/files/patch-src_vsp_af.cpp
  head/cad/openvsp/files/patch-src_vsp_havoc__geom.cpp
  head/cad/openvsp/files/patch-src_vsp_havoc__geom.h
Comment 20 Raphael Kubo da Costa freebsd_committer freebsd_triage 2016-02-25 22:22:35 UTC
Landed, thanks!
Comment 21 David Chisnall freebsd_committer freebsd_triage 2016-02-26 09:36:14 UTC
This patch looks more complicated than it needs to be.  Wouldn't inserting:

using ::array;

Immediately after the definition of array work?  Alternatively, changing the uses  of array to ::array would also work without sticking array in a namespace.
Comment 22 Raphael Kubo da Costa freebsd_committer freebsd_triage 2016-02-26 10:55:49 UTC
(In reply to David Chisnall from comment #21)
> This patch looks more complicated than it needs to be.  Wouldn't inserting:
> 
> using ::array;
> 
> Immediately after the definition of array work?

It doesn't seem to work:

% cat foo.cc
#include <iostream>
using namespace std;  // Not doing that here doesn't help, as it can be done indirectly from another file included before this one.
template<typename T>
struct array {
        array(T t);
};
using ::array;
template<typename T>
array<T>::array(T t) {}

% clang++ -c foo.cc
foo.cc:9:1: error: unknown type name 'array'
array<T>::array(T t) {}
^
foo.cc:9:6: error: expected unqualified-id
array<T>::array(T t) {}
     ^
2 errors generated.

> Alternatively, changing the
> uses  of array to ::array would also work without sticking array in a
> namespace.

That solves the issue for users of the array class, but not for array.h itself (plus the same amount of lines would need to be changed be it to "::array" or "openvsp_array".
Comment 23 David Chisnall freebsd_committer freebsd_triage 2016-02-26 11:01:20 UTC
I hadn't realised it had out-of-line definitions.  These also need prefixing with ::.  The following works, and restricts the changes to the declaration of the template:

#include <iostream>
using namespace std;
template<typename T>
struct array {
       array(T t);
};
using ::array;
template<typename T>
::array<T>::array(T t) {}

array<int> x(1);
Comment 24 Raphael Kubo da Costa freebsd_committer freebsd_triage 2016-02-26 11:11:43 UTC
(In reply to David Chisnall from comment #23)
> The following works

It still doesn't work here (clang++ -c test.cc, no additional parameters):

test.cc:11:1: error: unknown type name 'array'
array<int> x(1);
^
test.cc:11:6: error: expected unqualified-id
array<int> x(1);
     ^
2 errors generated.
Comment 25 David Chisnall freebsd_committer freebsd_triage 2016-02-26 11:13:42 UTC
Works for me:

$ clang++ -c foo.cc
$ clang++ -v
FreeBSD clang version 3.4.1 (tags/RELEASE_34/dot1-final 208032) 20140512
Target: x86_64-unknown-freebsd10.2
Thread model: posix
Selected GCC installation:
Comment 26 Raphael Kubo da Costa freebsd_committer freebsd_triage 2016-02-26 11:15:04 UTC
Well, that's FreeBSD 10.2, where the port was already working. The problem only happens in HEAD after a newer libc++ was imported.
Comment 27 Fernando Apesteguía freebsd_committer freebsd_triage 2016-02-26 11:15:39 UTC
(In reply to David Chisnall from comment #25)
In 10.2 and below OpenVSP compiles without these modifications. It is in 11-CURRENT where it started to fail when we landed the new clang/libc++
Comment 28 David Chisnall freebsd_committer freebsd_triage 2016-02-26 11:17:23 UTC
Ah, I see.  It would have helped if your reduced test case had used #include <array> instead of #include <iostream>.
Comment 29 Raphael Kubo da Costa freebsd_committer freebsd_triage 2016-02-26 11:20:42 UTC
I avoided using <array> because it does't exist before C++11, and this port is not built with -std=c++11.
Comment 30 Fernando Apesteguía freebsd_committer freebsd_triage 2016-02-27 10:30:19 UTC
(In reply to Raphael Kubo da Costa from comment #29)

I reported the problem upstream. At least to me, the response was a bit unexpected: libc++ doesn't support compiling c++98 programs that don't compile also as c++11 programs:

https://llvm.org/bugs/show_bug.cgi?id=26754

Than means that the any legacy program using a type as array, forward_list and probably a few more will clash with the ones in the std namespace for c++11 in libc++.

At least, we should reflect this fact in the documentation:
https://www.freebsd.org/doc/en/books/porters-handbook/book.html#uses-compiler
and maybe add a new knob in "USES= compiler:..." for cases like this.

Apparently GNU's C++ library is doing the right thing: they don't let c++11 names leak into std namespace unless c++11 standard is explicitly specified.