| Summary: | sys-libs/ncurses-5.9-r2 uses invalid C++ code [PATCH Included] | ||
|---|---|---|---|
| Product: | Gentoo/Alt | Reporter: | Richard Yao (RETIRED) <ryao> |
| Component: | FreeBSD | Assignee: | Gentoo's Team for Core System packages <base-system> |
| Status: | RESOLVED FIXED | ||
| Severity: | QA | CC: | dickey |
| Priority: | Normal | Keywords: | Bug, PATCH |
| Version: | unspecified | ||
| Hardware: | All | ||
| OS: | All | ||
| URL: | http://lists.gnu.org/archive/html/bug-ncurses/2012-05/msg00008.html | ||
| Whiteboard: | |||
| Package list: | Runtime testing required: | --- | |
| Bug Depends on: | |||
| Bug Blocks: | 417789 | ||
| Attachments: |
Patch to fix invalid C++ code in sys-libs/ncurses-5.9-r2
sys-libs/ncurses-5.9-r2 failure log sys-libs/ncurses-5.9-r2 build.log from Gentoo Prefix error messages using Debian/experimental clang 3.1-4 |
||
Created attachment 313257 [details]
sys-libs/ncurses-5.9-r2 failure log
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. Fix committed to CVS with Chainsaw's approval. 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. (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. 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.
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). For instance, using the suggested patch, the code does not build using clang++ 3.0-6, but does build using g++ 4.6.3 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. Created attachment 313991 [details]
error messages using Debian/experimental clang 3.1-4
(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? (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. (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. (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.) 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 (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. (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 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. (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. I see - then I'll verify the report and alternate fix (thanks). ncurses-5.9-20120608 is out with this patch included. Would base-system do a version bump? (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. (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. |
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.