Opened 8 years ago

Closed 8 years ago

#1773 closed defect (fixed)

build failure with clang++ on MacOS Lion

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20120417
Component: build system Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 2 Add Hours to Ticket: 0
Total Hours: 2.73 Internal?: no

Description

I suspect it's common for all variants of clang++ of the same version
(mine is 3.1), but Lion is the only OS I'm using that has this version
of clang++.

It fails as follows:

In file included from message_renderer_bench.cc:In file included from rdatarender_bench.cc:22:
../../../../src/lib/bench/benchmark.h:213:62: error: binding reference member
      'target_' to stack allocated parameter 'target' [-Werror,-Wdangling-field]
        iterations_(iterations), sub_iterations_(0), target_(target)
                                                             ^~~~~~
rdatarender_bench.cc:180:5: note: in instantiation of member function
      'isc::bench::BenchMark<<anonymous>::RdataRenderBenchMark<boost::shared_ptr<const
      isc::dns::rdata::Rdata> > >::BenchMark' requested here

I've already figured out how to avoid this (I'll soon commit a branch
containing the initial fix). The rest of the task is to complete it
(I guess it's just some minor cleanups), get it reviewed and merge.

Subtickets

Change History (14)

comment:1 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 2

comment:2 Changed 8 years ago by jinmei

  • Milestone changed from Year 3 Task Backlog to Next-Sprint-Proposed

(I would have proposed this for this sprint...)

comment:3 Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to jinmei
  • Status changed from new to accepted

comment:4 Changed 8 years ago by jinmei

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

comment:5 Changed 8 years ago by jinmei

(Moved from the proposed queue due to lack of new open tasks)

This task is ready for review at branch trac1773new (the original
trac1773 was based on an older master and was already pushed to the
public repo, so I've created a new one to minimize the risk of
confusion while using a newer base).

This is the proposed changelog entry:

418.	[build]		jinmei
	Made sure BIND 10 can be built with clang++ 3.1.  (It failed on
	MacOS 10.7 using Xcode 4.3, but it's more likely to be a matter of
	clang version.)
	(Trac #1773, git TBD)

comment:6 Changed 8 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:7 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:8 follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

First, should the target_ be initilazed to NULL here?

     BenchMark(const int iterations, T target) :
-        iterations_(iterations), sub_iterations_(0), target_(target)
+        iterations_(iterations), sub_iterations_(0)
     {
-        initialize(true);
+        initialize(target, true);
     }

Also, I'm not sure this is the correct fix. I guess this happened:

  • The parameter of the constructor is T, not T& here.
  • If left for autodetection, it may have happened gcc chose Type& as T and clang++ only Type. Anyway, the T got expanded to the bare type.
  • As such, the parameter is passed by copy allocated on the stack. The reference should be taken from the parameter on the stack which is obviously wrong, as the parameter ceases to exist soon.

I believe the correct fix in this case is not this workaround (because then the copied parameter would be initialized instead of the correct one anyway and it needs the type to be copy-constructible), but changing the constructor to T& target instead of T target.

Also, did you notice the commit message has two [branch] tags?

comment:9 in reply to: ↑ 8 ; follow-up: Changed 8 years ago by jinmei

Replying to vorner:

Hello

First, should the target_ be initilazed to NULL here?

     BenchMark(const int iterations, T target) :
-        iterations_(iterations), sub_iterations_(0), target_(target)
+        iterations_(iterations), sub_iterations_(0)
     {
-        initialize(true);
+        initialize(target, true);
     }

Ah, yes, that's better (target_ shouldn't be used in the intended code
path, but it's always a good practice to initialize everything).

Also, I'm not sure this is the correct fix. I guess this happened:

  • The parameter of the constructor is T, not T& here.
  • If left for autodetection, it may have happened gcc chose Type& as T and clang++ only Type. Anyway, the T got expanded to the bare type.
  • As such, the parameter is passed by copy allocated on the stack. The reference should be taken from the parameter on the stack which is obviously wrong, as the parameter ceases to exist soon.

No, if the compiler consider T = Type& it wouldn't compile whether
it's g++ or clang++ because in our usage target is a temporary
object and this is a non const reference. The following code
shouldn't compile either by g++ or clang++:

struct Foo {
    explicit Foo(int&) {}
    //explicit Foo(int) {}
};

int
main() {
    Foo(10);
    return (0);
}

If we re-enabled the commented-out constructor, it should compile, and
the second one should always be chosen because that's the only legal
choice.

As such,

I believe the correct fix in this case is not this workaround (because then the copied parameter would be initialized instead of the correct one anyway and it needs the type to be copy-constructible), but changing the constructor to T& target instead of T target.

this cannot be a solution. Using const T& target instead of
T target would avoid this particular issue, but we cannot declare
target as const because we need to be able to call non const member
function of it (run()).

I admit my workaround may not seem very clean, and if there's a
workable alternative, I'm happy to use that. But merely changing the
argument type to a reference is not a workable solution.

Also, did you notice the commit message has two [branch] tags?

Ah, I didn't, but I now see it and I know why. It should be
automatically done when I cherry-picked it from the older branch.
That should be a one time garbage.

comment:10 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:11 in reply to: ↑ 9 ; follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to jinmei

Replying to jinmei:

If we re-enabled the commented-out constructor, it should compile, and
the second one should always be chosen because that's the only legal
choice.

Hmm, then it would have to be called as:

        int i(10);
        Foo(i);

Which is longer.

this cannot be a solution. Using const T& target instead of
T target would avoid this particular issue, but we cannot declare
target as const because we need to be able to call non const member
function of it (run()).

Thinking about it, C++ warnings and rules here are a terrible mess. Why keeping a const reference should be safer than keeping a non-const one is a mystery. Anyway, if the goal is to be able to just call the BenchMark like a function on something where the run() is non-const, then your solution is probably the way to go.

So if changing the run() to const or calling it with pre-creating the variable is not an (better) option, then I think it should be merged.

comment:12 Changed 8 years ago by vorner

  • Total Hours changed from 0 to 0.73

comment:13 in reply to: ↑ 11 Changed 8 years ago by jinmei

Replying to vorner:

Replying to jinmei:

If we re-enabled the commented-out constructor, it should compile, and
the second one should always be chosen because that's the only legal
choice.

Hmm, then it would have to be called as:

        int i(10);
        Foo(i);

Which is longer.

Right, and the second constructor that takes a reference in case the
redundant setup is acceptable in favor of avoiding the copy. But in
the benchmark cases I believe conciseness would be much more
preferred.

this cannot be a solution. Using const T& target instead of
T target would avoid this particular issue, but we cannot declare
target as const because we need to be able to call non const member
function of it (run()).

Thinking about it, C++ warnings and rules here are a terrible mess. Why keeping a const reference should be safer than keeping a non-const one is a mystery. Anyway, if the goal is to be able to just call the BenchMark like a function on something where the run() is non-const, then your solution is probably the way to go.

So if changing the run() to const or calling it with pre-creating the variable is not an (better) option, then I think it should be merged.

Okay. Pre-creating the variable *could be* an option, but I suspect
it's less preferred as commented above. I'm afraid changing run to
const cannot be an option since it'd be more reasonable to assume
type T contains some mutable state and run() could modify it (which is
the case for some of our benchmark test code).

So I'm going to merge the current branch. After all, this is a helper
internal tool, and we can quite freely revisit the design if we come
up with a better approach.

comment:14 Changed 8 years ago by jinmei

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