Opened 7 years ago

Closed 7 years ago

#2340 closed defect (fixed)

build failures with newer versions of clang++

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20121023
Component: build system 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: 2.5 Internal?: no

Description

I noticed this while I tried to build BIND 10 on Mac OS X 10.8.
As a result of upgrading the OS I ended up upgrading Xcode, which
installed a bleeding edge version of clang.

It caused a few new build failures:

  • clang is now more strict about "unused variables" (some of which I suspect are mostly false positives).
  • it's more sensitive to the "driver" (such as compiler or linker) argument (options), and treats some of them as a fatal error with -Werror. Specifically, the -pthread option specified some part of the BIND 10 code caused fatal build errors. It also made it possible to use clang++ via ccache.

Fixing or working around the first issue was easy. For the second
issue, I propose disabling the warning by specifying
-Qunused-arguments. While it may be possible to carefully set and
position compiler options to avoid the issue, I'm afraid it can soon
repeat as we support multiple compilers. Suppressing warnings is
generally bad, but in this case it's at least irrelevant to ensuring
the code quality.

I have a private branch implementing the proposal. I'm going to push
it and move this ticket to the review queue.

Subtickets

Change History (11)

comment:1 Changed 7 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20121023

comment:2 Changed 7 years ago by jinmei

trac2340 is ready for review.

This consists of two sets of independent changes:

  • 23da2f0 and 9ea5133: this suppresses "unused variable" warnings from newer versions of clang(++). I suspect the former might rather be a clang bug (it seems to happen only in some template context), but we wouldn't like to suppress the entire warning just for this. I've added a comment about why we need this at 28a6c71.
  • c629dec: this is to address the "unused (driver) arguments" problem. I made some piggy-back cleanup to unify compiler-specific settings, which makes the diff larger than the absolute minimum, but the essential change is only this:
    +# Newer versions of clang++ promotes "unused driver arguments" warnings to
    +# a fatal error with -Werror, causing build failure.  Since we use multiple
    +# compilers on multiple systems, this can easily happen due to settings for
    +# non clang++ environments that could be just ignored otherwise.  It can also
    +# happen if clang++ is used via ccache.  So, although probably suboptimal,
    +# we suppress this particular warning.  Note that it doesn't weaken checks
    +# on the source code.
    +if test "$CLANGPP" = "yes"; then
    +       B10_CXXFLAGS="$B10_CXXFLAGS -Qunused-arguments"
    +fi
    

This is the proposed changelog:

487.?	[build]		jinmei
	Fixed build failure with newer versions of clang++.  These
	versions are stricter regarding "unused variable" and "unused
	(driver) arguments" warnings, and cause fatal build error
	with -Werror.  The affected versions of clang++ include Apple's
	customized version 4.1 included in Xcode 4.5.1.  So this fix
	will solve build errors for Mac OS X that uses newer versions of
	Xcode.
	(Trac #2340, git TBD)

comment:3 Changed 7 years ago by jinmei

  • Status changed from new to reviewing

comment:4 Changed 7 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:5 follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to jinmei

Regarding the change in memory_datasrc_link.cc, I see why it would work, but I do wonder if there is also not some static check that flags using methods or constructors that don't put the result somewhere :) (of course, if so, it's easy to fix as well)

The problem in the scoped_lock one really does seem like a checker bug btw

Changes look OK, but I must admit I have not tried building it with clang myself yet.

In effect, the changes to configure.ac are small, but shall we run it through buildbot first just in case?

comment:6 Changed 7 years ago by jelte

Oh, two more things

  • as-is, this branch does not compile on my system, but a recent change in master fixes that (LDADD in datasrc/memory makefile). This may also happen on other systems, so if you want to run this to buildbot I suggest making a new branch from master, merge this into that new branch, and run that.
  • the commit logs do not have this ticket number in them, but they have [clang-build]

comment:7 in reply to: ↑ 5 Changed 7 years ago by jinmei

Replying to jelte:

Regarding the change in memory_datasrc_link.cc, I see why it would work, but I do wonder if there is also not some static check that flags using methods or constructors that don't put the result somewhere :) (of course, if so, it's easy to fix as well)

I think such warnings normally doesn't happen because there's always
possibility that a call to the constructor has a side effect (but of
course there can be a tool that even checks the internal of the
constructor). In this particular case, I ran the branch on buildbots
(including the one running cppcheck, I believe) and didn't get a
failure report; cppcheck also passed locally.

So, at least for now I think it's okay to keep this code. As you
said, if we encounter a new alarm due to that, we can then consider
how to work around it.

The problem in the scoped_lock one really does seem like a checker bug btw

Changes look OK, but I must admit I have not tried building it with clang myself yet.

In effect, the changes to configure.ac are small, but shall we run it through buildbot first just in case?

That was indeed a good idea, and there was indeed a bug identified by
a SunStudio build. It's fixed at b10b39f. I also merge a newer
version of master to this branch. With these it passed on all
buildbots. So I believe it will pass on your environment, too.

Due to the merge with master, getting the diff is difficult, but the
only change I made on the branch is b10b39f. So could you just check
that particular commit?

Regarding [clang-build], yes I know that, it's due to an earlier local
attempt. I didn't touch them as I thought the difference is minor,
but if you want I'll override it by git rebase.

comment:8 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:9 follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to jinmei

there is one problem in that commit;
configure.ac:363 should not have 'test' after the -a

When that is fixed it can be merged.

Regarding clang-build, I'd personally prefer we stick to the convention of commits, to make it easier to trac[k] down why commits were made, but no very strong opinion in this specific case, and I'll leave it up to you.

comment:10 in reply to: ↑ 9 Changed 7 years ago by jinmei

Replying to jelte:

there is one problem in that commit;
configure.ac:363 should not have 'test' after the -a

When that is fixed it can be merged.

Oops, thanks for catching that. I don't understand how I could have
pushed it.

Regarding clang-build, I'd personally prefer we stick to the convention of commits, to make it easier to trac[k] down why commits were made, but no very strong opinion in this specific case, and I'll leave it up to you.

I've reworded the commit logs using rebase -i to replace [clang-build]
with [2340]. But maybe due to the unusual workflow with intermediate
merge, etc, the resulting history may not be very helpful.

Anyway, I've merged the branch to master, and am closing the ticket.

Thanks,

comment:11 Changed 7 years ago by jinmei

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 2.5
Note: See TracTickets for help on using tickets.