Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 417763 - sys-libs/ncurses-5.9-r2 uses invalid C++ code [PATCH Included]
Summary: sys-libs/ncurses-5.9-r2 uses invalid C++ code [PATCH Included]
Status: RESOLVED FIXED
Alias: None
Product: Gentoo/Alt
Classification: Unclassified
Component: FreeBSD (show other bugs)
Hardware: All All
: Normal QA (vote)
Assignee: Gentoo's Team for Core System packages
URL: http://lists.gnu.org/archive/html/bug...
Whiteboard:
Keywords: Bug, PATCH
Depends on:
Blocks: freebsd-clang-3.1
  Show dependency tree
 
Reported: 2012-05-27 13:37 UTC by Richard Yao
Modified: 2012-06-13 19:47 UTC (History)
1 user (show)

See Also:
Package list:
Runtime testing required: ---


Attachments
Patch to fix invalid C++ code in sys-libs/ncurses-5.9-r2 (ncurses-5.9-fix-clang-build.patch,1.26 KB, patch)
2012-05-27 13:37 UTC, Richard Yao
Details | Diff
sys-libs/ncurses-5.9-r2 failure log (build.log,649.53 KB, text/plain)
2012-05-27 13:38 UTC, Richard Yao
Details
sys-libs/ncurses-5.9-r2 build.log from Gentoo Prefix (build.log,664.26 KB, text/plain)
2012-05-31 21:17 UTC, Richard Yao
Details
error messages using Debian/experimental clang 3.1-4 (with-patch-debian-exp.log,2.26 KB, text/plain)
2012-06-02 23:39 UTC, Thomas Dickey
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Yao gentoo-dev 2012-05-27 13:37:41 UTC
Created attachment 313255 [details, diff]
Patch to fix invalid C++ code in sys-libs/ncurses-5.9-r2

Compiling ncurses with Clang revealed that it has illegal C++ code. I have attached a patch for this issue. I have also emailed it to upstream.
Comment 1 Richard Yao gentoo-dev 2012-05-27 13:38:11 UTC
Created attachment 313257 [details]
sys-libs/ncurses-5.9-r2 failure log
Comment 2 Richard Yao gentoo-dev 2012-05-27 13:38:54 UTC
As an additional comment, it might be worthwhile to support epatch_user. I tried using it when developing this patch, but the ebuild does not support it.
Comment 3 Richard Yao gentoo-dev 2012-05-29 08:27:05 UTC
Fix committed to CVS with Chainsaw's approval.
Comment 4 SpanKY gentoo-dev 2012-05-29 16:18:12 UTC
did you post this to the upstream mailing list yet ?  i don't want to be carrying a lot of clang patches.  especially when the patches affect header files actually installed by the packages.
Comment 5 Richard Yao gentoo-dev 2012-05-29 20:06:04 UTC
(In reply to comment #4)
> did you post this to the upstream mailing list yet ?  i don't want to be
> carrying a lot of clang patches.  especially when the patches affect header
> files actually installed by the packages.

I emailed it to bug-ncurses@gnu.org two days ago.
Comment 6 Richard Yao gentoo-dev 2012-05-31 21:17:36 UTC
Created attachment 313757 [details]
sys-libs/ncurses-5.9-r2 build.log from Gentoo Prefix

This failure also occurs on Gentoo Prefix. It seems that the Prefix overlay has a fork of the ebuild. I am CCing the Prefix team.
Comment 7 Thomas Dickey 2012-06-02 20:35:01 UTC
This is "old" code, which has been built with several compilers.
On Debian/testing I have for instance clang++ 3.0-6, where the
code compiles without a problem.  Given that the log shows "3.1",
it's also possible that the bug lies in clang (and that is what
I am investigating).
Comment 8 Thomas Dickey 2012-06-02 20:46:55 UTC
For instance, using the suggested patch, the code does not build
using clang++ 3.0-6, but does build using g++ 4.6.3
Comment 9 Thomas Dickey 2012-06-02 23:34:28 UTC
I found that my Debian/experimental has clang 3.1-4
However, that version does not compile with the given patch.
It successfully compiles the unpatched version (current
ncurses development version).  By the way, I did make
different fixes to the c++ binding in April 2011.  But
lacking something more definite (a clear indication that
clang is now reporting something that other compilers have
been ignoring), there's nothing that I can fix.
Comment 10 Thomas Dickey 2012-06-02 23:39:14 UTC
Created attachment 313991 [details]
error messages using Debian/experimental clang 3.1-4
Comment 11 Richard Yao gentoo-dev 2012-06-03 00:01:14 UTC
(In reply to comment #7)
> This is "old" code, which has been built with several compilers.
> On Debian/testing I have for instance clang++ 3.0-6, where the
> code compiles without a problem.  Given that the log shows "3.1",
> it's also possible that the bug lies in clang (and that is what
> I am investigating).

Are you applying any patches to Ncurses on Debian?
Comment 12 Richard Yao gentoo-dev 2012-06-03 00:29:26 UTC
(In reply to comment #10)
> Created attachment 313991 [details]
> error messages using Debian/experimental clang 3.1-4

After examining this more carefully, I am convinced that there is a patch being applied to ncurses on your system:

In file included from ../c++/cursesf.cc:35:
../c++/cursesf.h:684:7: error: no matching constructor for initialization of 'NCursesForm'
    : NCursesForm (&Fields, with_frame, autoDelete_Fields) {
      ^            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../c++/cursesf.h:420:3: note: candidate constructor not viable: no known conversion from 'NCursesFormField ***' to 'NCursesFormField **' for 1st argument; remove &

The Fields variable lacks a deference operator on Gentoo, but it has one on your system, which causes the failure. I suspect that someone else tried to fix this by adding the & operator. Doing that is wrong because & will provide a pointer to the stack value. The correct solution should be to change the type of the constructor parameter as I did.
Comment 13 Richard Yao gentoo-dev 2012-06-03 00:37:13 UTC
(In reply to comment #12)
> The Fields variable lacks a deference operator on Gentoo, but it has one on
> your system, which causes the failure. I suspect that someone else tried to
> fix this by adding the & operator. Doing that is wrong because & will
> provide a pointer to the stack value. The correct solution should be to
> change the type of the constructor parameter as I did.

On second thought, I might be wrong here. The result of & inside a contructor's member initializer list might be undefined, which could mean that GCC is providing a proper pointer rather than a stack reference. This does merit further investigation.

The open question is whether or not the compilers are doing the right thing in accepting that in the first place. If that is acceptable, then a further question would be why do ncurses programs work with it. i.e. are the compilers treating &Fields in the constructor list as meaning "treat this variable as if it were a reference and dereference it" or is the code constructed in a way in which a stack pointer value here doesn't matter.

Either way, I am certain that a patch is being applied on your end to introduce that & and I believe that it should be reverted.
Comment 14 Fabian Groffen gentoo-dev 2012-06-03 08:41:52 UTC
(In reply to comment #6)
> Created attachment 313757 [details]
> sys-libs/ncurses-5.9-r2 build.log from Gentoo Prefix
> 
> This failure also occurs on Gentoo Prefix. It seems that the Prefix overlay
> has a fork of the ebuild. I am CCing the Prefix team.

The only thing I can add here, but seems unrelated is that -D_XOPEN_SOURCE=500 breaks compilation on Solaris with GCC >=4.6, so I had to remove that from c++/Makefile (and only that file, we need it for C-code).

(The GCC compiler on Solaris defines _XOPEN_SOURCE=600 since 4.6, _XOPEN_SOURCE=500 previously.  This to match standards' specifications.)
Comment 15 Thomas Dickey 2012-06-03 14:33:41 UTC
well... my own builds are plain-vanilla (no patches, just different
configure options).  Repeating myself, I made fixes for clang 3.0
just after 5.9-release, and to make progress on this report, it would
help for Gentoo packagers to start looking at those fixes.

Regarding _XOPEN_SOURCE, likewise I made fixes last fall to consolidate
changes across a number of platforms.   There's a roll-up patch on my
website which you would find useful as a starting point for both of these
issues.

You might find this helpful:
http://invisible-island.net/ncurses/NEWS.html
Comment 16 Fabian Groffen gentoo-dev 2012-06-03 15:31:44 UTC
(In reply to comment #15)
> You might find this helpful:
> http://invisible-island.net/ncurses/NEWS.html

Feels like we need _px releases applying all the weekly patches, or take a snapshot from http://ncurses.scripts.mit.edu/?p=ncurses.git;a=summary.  I'll leave a decision here up to base-system.  I see no business for prefix in here any more.
Comment 17 Tim Harder gentoo-dev 2012-06-03 16:47:39 UTC
(In reply to comment #16)
> Feels like we need _px releases applying all the weekly patches, or take a
> snapshot from http://ncurses.scripts.mit.edu/?p=ncurses.git;a=summary.  I'll
> leave a decision here up to base-system.  I see no business for prefix in
> here any more.

It seems that we could just use the latest weekly snapshot tarballs located here [1].

[1]: ftp://invisible-island.net/ncurses/current
Comment 18 SpanKY gentoo-dev 2012-06-04 00:54:41 UTC
i stopped including the snapshots as they got to be too much of a hassle.  if there's particular fixes to include, then post them and we can include them.
Comment 19 Richard Yao gentoo-dev 2012-06-04 00:58:23 UTC
(In reply to comment #18)
> i stopped including the snapshots as they got to be too much of a hassle. 
> if there's particular fixes to include, then post them and we can include
> them.

I don't advise that. The upstream developer replied to my email on the list explaining the this was fixed in the December rollup patch. Unfortunately, that patch addresses this by taking the address of a temporary variable on the stack and then storing it persistently for later access, which is wrong.

I am adding the upstream developer to the CC list. This needs further discussion.
Comment 20 Thomas Dickey 2012-06-04 10:58:57 UTC
I see - then I'll verify the report and alternate fix (thanks).
Comment 21 Richard Yao gentoo-dev 2012-06-11 00:48:23 UTC
ncurses-5.9-20120608 is out with this patch included. Would base-system do a version bump?
Comment 22 SpanKY gentoo-dev 2012-06-11 19:34:12 UTC
(In reply to comment #21)

as i mentioned in comment #18, i'm not keen on trying to do snapshot releases again.  if you cut out the patch and attach here, then including it in the existing version is fine.
Comment 23 Richard Yao gentoo-dev 2012-06-13 19:47:45 UTC
(In reply to comment #22)
> (In reply to comment #21)
> 
> as i mentioned in comment #18, i'm not keen on trying to do snapshot
> releases again.  if you cut out the patch and attach here, then including it
> in the existing version is fine.

Upstream merged the patch that I committed on 2012-05-27, so there is nothing more for us to do here. Closing as fixed.