Opened 6 years ago

Closed 6 years ago

#3112 closed defect (fixed)

boost::optional failure on Debian i686

Reported by: muks Owned by: muks
Priority: high Milestone: Sprint-20131001
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 3 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

boost::optional from Boost 1.54 fails compile on Debian i686 builder. See:

http://git.bind10.isc.org/~tester/builder/BIND10/20130822021503-Debian6Linux-i686-GCC/logs/build.out

/lib/dns -I../../../src/lib/util -I../../../src/lib/util -I/home/jreed/boost_1_54_0 -DOS_LINUX  -I../../../ext/asio -I../../../ext/coroutine -DASIO_DISABLE_THREADS=1 -Wall -Wextra -Wnon-virtual-dtor -Wwrite-strings -Woverloaded-virtual -Wno-sign-compare -Werror -fPIC -g -O2 -MT libb10_dns___la-master_loader.lo -MD -MP -MF .deps/libb10_dns___la-master_loader.Tpo -c -o libb10_dns___la-master_loader.lo `test -f 'master_loader.cc' || echo './'`master_loader.cc
libtool: compile:  g++ -DHAVE_CONFIG_H -I. -I../../.. -I../../../src/lib -I../../../src/lib -I../../../src/lib/dns -I../../../src/lib/dns -I../../../src/lib/util -I../../../src/lib/util -I/home/jreed/boost_1_54_0 -DOS_LINUX -I../../../ext/asio -I../../../ext/coroutine -DASIO_DISABLE_THREADS=1 -Wall -Wextra -Wnon-virtual-dtor -Wwrite-strings -Woverloaded-virtual -Wno-sign-compare -Werror -fPIC -g -O2 -MT libb10_dns___la-master_loader.lo -MD -MP -MF .deps/libb10_dns___la-master_loader.Tpo -c master_loader.cc  -fPIC -DPIC -o .libs/libb10_dns___la-master_loader.o
cc1plus: warnings being treated as errors
/home/jreed/boost_1_54_0/boost/optional/optional.hpp: In member function 'isc::dns::RRType isc::dns::MasterLoader::MasterLoaderImpl::parseRRParams(bool&, isc::dns::MasterToken)':
/home/jreed/boost_1_54_0/boost/optional/optional.hpp:433: error: dereferencing pointer 'pretmp.3987' does break strict-aliasing rules
cc1plus: note: initialized from here
/home/jreed/boost_1_54_0/boost/optional/optional.hpp:346: error: dereferencing pointer 'pretmp.3987' does break strict-aliasing rules
cc1plus: note: initialized from here

Subtickets

Change History (18)

comment:1 Changed 6 years ago by muks

Also see #2593.

comment:2 follow-up: Changed 6 years ago by muks

See: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49019

What version of GCC is used on this Debian machine?

comment:3 in reply to: ↑ 2 Changed 6 years ago by jreed

Replying to muks:

What version of GCC is used on this Debian machine?

g++ (Debian 4.4.5-8) 4.4.5

ii g++ 4:4.4.5-1 The GNU C++ compiler
ii g++-4.4 4.4.5-8 The GNU C++ compiler
ii gcc 4:4.4.5-1 The GNU C compiler
ii gcc-4.4 4.4.5-8 The GNU C compiler
ii gcc-4.4-base 4.4.5-8 The GNU Compiler Collection (base package)
ii libgcc1 1:4.4.5-8 GCC support library
ii libgomp1 4.4.5-8 GCC OpenMP (GOMP) support library

comment:4 Changed 6 years ago by muks

  • Estimated Difficulty changed from 0 to 3

Adding sole estimation, but it looks unlikely we can fix this.

comment:5 Changed 6 years ago by muks

We decided on the sprint planning call to remove the use of boost::optional to avoid this issue.

comment:6 Changed 6 years ago by muks

  • Milestone changed from Next-Sprint-Proposed to Sprint-20130917
  • Priority changed from medium to high

Moving to current sprint after discussion during the team call. Also raising priority.

comment:7 follow-up: Changed 6 years ago by kean

Just FYI this only causes a build failure because of -Werror. No sane compiler would make aliasing issues an error and gcc/g++ certainly doesn't. It's hackinh as hell but if there is no other way of avoiding the warning there is always the distasteful fallback option of using GCC's pragmas's to stifle the warning. Or to turn off -Werror for this file. Last option though, it is obviously better to eliminate the aliased pointer in the first place.

comment:8 Changed 6 years ago by muks

  • Status changed from new to reviewing

Up for review.

comment:9 Changed 6 years ago by muks

A DHCP fix commit was added for a compiler warning which showed up after libdns++ compiled and it proceeded to build the DHCP libraries. The fix was reviewed by Thomas.

This passed build on Debian i686 now:
http://git.bind10.isc.org/~tester/builder/BIND10/20130912144044-Debian6Linux-i686-GCC/logs/

comment:10 Changed 6 years ago by muks

This does not need a ChangeLog entry as there is no externally visible change.

comment:11 in reply to: ↑ 7 Changed 6 years ago by vorner

Replying to kean:

Just FYI this only causes a build failure because of -Werror. No sane compiler would make aliasing issues an error and gcc/g++ certainly doesn't. It's hackinh as hell but if there is no other way of avoiding the warning there is always the distasteful fallback option of using GCC's pragmas's to stifle the warning. Or to turn off -Werror for this file. Last option though, it is obviously better to eliminate the aliased pointer in the first place.

Not directly related to the topic of the ticket, but just for the record, I'm strongly opposed to silencing this warning. While warning about assignment inside conditional or unused variable is harmless, this one should really read as „I may (and probably will) generate bogus code“. The compiler may assume that pointers that are not allowed to alias are distinct and base some optimisations on it (and it does). If this is not true, the resulting code can break quite badly.

There's option that turns off not only the warning, but also this kind of optimisations.

comment:12 Changed 6 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:13 Changed 6 years ago by vorner

  • Owner changed from vorner to muks

Hello

First, I tried to reproduce on a debian I installed into a virtual machine. I didn't get the error, so I'm not sure it helped, but I guess you tried on that machine.

The other thing, this comment with two copy constructors:

    /// \brief Copy constructor.
    ///
    /// This constructor never throws an exception.

This could lead people to believe that this bit of code could never throw:

RRTTL *ttl = new RRTTL(other_ttl);

This is not true (it can throw std::bad_alloc), even though the comment is not a lie strictly speaking, because the thing that throws is not the constructor itself. May I suggest either removing the comment about exceptions completely or restating it somehow?

Other than that, I think it can be merged.

comment:14 Changed 6 years ago by muks

  • Owner changed from muks to vorner

Hi Michal

I simply removed the copy constructors (they were added in this branch) to use the default ones instead. Another commit to avoid unnecessary construction in two places was also pushed.

Please can you review these?

comment:15 Changed 6 years ago by vorner

  • Owner changed from vorner to muks

Hello

Not that I'd be directly opposed to the last commit which avoids some constructions, but it looks more complex. It would be nice to include a comment about what it does, because it needs some non-trivial parsing.

Please merge then.

comment:16 Changed 6 years ago by muks

  • Owner changed from muks to vorner

I have made a wrapper of the assignment code as there were 3 copies of it in the master loader. Please can you review it?

comment:17 Changed 6 years ago by vorner

  • Owner changed from vorner to muks

Yes, it seems OK.

comment:18 Changed 6 years ago by muks

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged to master branch in commit e33d14843af79aa49e86fe4f13b8e253fcbe7fb2:

* ab67242 [3112] Assign the TTL objects using a helper function
* dd4f121 [3112] Avoid construction in some places
* 9b98a01 [3112] Remove the copy-constructors, favoring default ones
* 384b365 [3112] Fix an uninit variable use warning
* cca46d0 [3112] Test the contents of RRClass returned by RRClass::createFromText()
* d8a6c3d [3112] Don't use boost::optional in libdns++

There is no need of a ChangeLog message.

Resolving as fixed. Thank you for the reviews Michal.

Note: See TracTickets for help on using tickets.