New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test/osdc: fix comparison error and silence warning from -Wunused-value #15353

Merged
merged 1 commit into from Jun 3, 2017

Conversation

Projects
None yet
5 participants
@wjwithagen
Contributor

wjwithagen commented May 29, 2017

Clang complains:

  /home/jenkins/workspace/ceph-freebsd/src/test/osdc/object_cacher_stress.cc:50:26: error: ordered comparison between pointer and zero ('std::atomic<unsigned int> *' and 'int')
      assert(m_outstanding > 0);
               ~~~~~~~~~~~~~ ^ ~
  /home/jenkins/workspace/ceph-freebsd/src/include/assert.h:117:5: note: expanded from macro 'assert'
    ((expr)                                                               \
        ^~~~

The following warning appears during build:

ceph/src/test/osdc/object_cacher_stress.cc: In member function ‘virtual void C_Count::finish(int)’:
ceph/src/test/osdc/object_cacher_stress.cc:51:5: warning: value computed is not used [-Wunused-value]
*m_outstanding--;

Signed-off-by: Willem Jan Withagen wjw@digiware.nl

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented May 30, 2017

/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/erasure-code/test-erasure-eio.sh:85: rados_put:  rados --pool pool-jerasure put obj-eio-10542-0 td/test-erasure-eio/ORIGINAL
/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/erasure-code/test-erasure-eio.sh: line 74:  8392 Terminated              rados --pool $poolname put $objname $dir/ORIGINAL
/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/erasure-code/test-erasure-eio.sh:85: rados_put:  return 1
/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/erasure-code/test-erasure-eio.sh:119: rados_put_get:  return 1
/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/erasure-code/test-erasure-eio.sh:173: rados_get_data_eio:  return 1
/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/erasure-code/test-erasure-eio.sh:314: TEST_rados_get_with_subreadall_eio_shard_0:  return 1

retest this please.

@joscollin

This comment has been minimized.

Member

joscollin commented May 30, 2017

Jenkins retest this please

@joscollin

joscollin requested changes May 30, 2017 edited

This change made by @wjwithagen may fix the issue reported by him in this PR. But it seems the value computed by *m_outstanding-- is not used. So I request a review from @chardan, who has implemented this.

@@ -47,7 +47,7 @@ class C_Count : public Context {
: m_op(op), m_outstanding(outstanding) {}
void finish(int r) override {
m_op->done++;
assert(m_outstanding > 0);
assert(*m_outstanding > 0);
*m_outstanding--;

This comment has been minimized.

@joscollin

joscollin May 30, 2017

Member

ceph/src/test/osdc/object_cacher_stress.cc: In member function ‘virtual void C_Count::finish(int)’:
ceph/src/test/osdc/object_cacher_stress.cc:51:5: warning: value computed is not used [-Wunused-value]
*m_outstanding--;

This comment has been minimized.

@chardan

chardan May 30, 2017

Contributor

+1 Yes, this fix looks correct. Good catch!

This comment has been minimized.

@joscollin

joscollin May 31, 2017

Member

@chardan The fix done by @wjwithagen is good and it eliminates the comparison warning. But It again gives the below warning for me. So when I reviewed the code, I have noticed that m_outstanding is just used for asserting and the decremented value is not used at all. So is it implemented just for asserting ? Is it an unwanted member variable that we can remove ? These are the doubts that I have. Please clarify the purpose of implementing it. Thanks.

ceph/src/test/osdc/object_cacher_stress.cc: In member function ‘virtual void C_Count::finish(int)’:
ceph/src/test/osdc/object_cacher_stress.cc:51:5: warning: value computed is not used [-Wunused-value]
*m_outstanding--;

This comment has been minimized.

@chardan

chardan May 31, 2017

Contributor

Hi, @joscollin, I don't see any other use for it. On line 71, the pointee variable "outstanding_reads" is declared, and then on 104 it's incremented, only to be decremented here when finish() is called. It looks like the assertion is checking that there's at least one outstanding read, but what I /don't/ see is a check to see if the final value is again zero after all operations are complete. I don't really see any use for it, since it's never going to be < 0 as far as I can tell, since nothing else appears to decrement it and it's always incremented before the construction of a new C_Count object. It could be that there's a side-effect I'm missing.

However, I didn't write the original code, so I'm also second-guessing the author. :-\

This comment has been minimized.

@joscollin

joscollin Jun 1, 2017

Member

@chardan When I did $git blame, it showed up your name. So that's the reason I contacted you.

As I analyse the code once again now, it is maintaining a count of outstanding_reads. When the C_Count object is created it is incremented and the m_outstanding is initialized. Then upon complete(), it calls finish() and the value is decremented. The only usage of the outstanding_reads is for that assert and the assert is just a check before decrementing. So basically there is no need for maintaining such a count, unless it is used for something other than asserting.

@wjwithagen However, I'm working on this to see if we can keep this variable and just avoid the two warnings that we have. I will let you know my update in the evening IST, as the build takes too long in my machine. :-)

This comment has been minimized.

@chardan

chardan Jun 1, 2017

Contributor

@joscollin Yeah, not sure why (I must have touched it, sometime! :>), but either way, happy to be involved. I agree with your analysis, that's what I found as well.

@@ -47,7 +47,7 @@ class C_Count : public Context {
: m_op(op), m_outstanding(outstanding) {}
void finish(int r) override {
m_op->done++;
assert(m_outstanding > 0);
assert(*m_outstanding > 0);
*m_outstanding--;

This comment has been minimized.

@joscollin

joscollin May 31, 2017

Member

@chardan The fix done by @wjwithagen is good and it eliminates the comparison warning. But It again gives the below warning for me. So when I reviewed the code, I have noticed that m_outstanding is just used for asserting and the decremented value is not used at all. So is it implemented just for asserting ? Is it an unwanted member variable that we can remove ? These are the doubts that I have. Please clarify the purpose of implementing it. Thanks.

ceph/src/test/osdc/object_cacher_stress.cc: In member function ‘virtual void C_Count::finish(int)’:
ceph/src/test/osdc/object_cacher_stress.cc:51:5: warning: value computed is not used [-Wunused-value]
*m_outstanding--;
@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jun 1, 2017

@joscollin
Now what if you move the -- within the assert?
assert(*m_outstanding-- > 0);
Especially since it is not used at other locations?

Then you could get an unused variable warning, when compiling with NDEBUG
But that is fixable in the code.

@chardan

This comment has been minimized.

Contributor

chardan commented Jun 1, 2017

I believe you'll need predecrement there to have the same effect.

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jun 1, 2017

@chardan
Why do you think that.
The decrement is now also after the assert?

@joscollin

This comment has been minimized.

Member

joscollin commented Jun 2, 2017

I believe you'll need predecrement there to have the same effect.
assert(*m_outstanding-- > 0);

@chardan @wjwithagen Now there is no use of assert. The decrement should happen only after the assert and the assert guarantees that the value of *m_outstanding is greater than 0 before decrementing.

As per my analysis,
In this line *m_outstanding--;, the precedence of postfix -- is higher than *
So the correct code should be (*m_outstanding)--;. This decrements the value pointed to by m_outstanding and it eliminates the warning: value computed is not used.

@wjwithagen As per my latest build, your warning: ordered comparison between pointer and zero magically disappears in the latest code for me. I update the code daily and upgrade my Fedora/Rawhide daily. Is there any fix in the latest ceph code not to check this ? However, you can include your fix, as it appears as an Error in FreeBSD: assert(*m_outstanding > 0); Anyway, it doesn't make sense to check if a pointer is greater than 0.

@liewegas

This comment has been minimized.

Member

liewegas commented Jun 2, 2017

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jun 2, 2017

@liewegas
Make sense.
@joscollin
So we can commit this?
Warnings in this case are understood, and can be ignored.

@joscollin

This comment has been minimized.

Member

joscollin commented Jun 2, 2017

@wjwithagen Yes. As far as I analysed, you can fix that line to (*m_outstanding)--; along with your own initial fix.

assert(*m_outstanding > 0);
(*m_outstanding)--;

This eliminates both warnings (or errors) and the logic seems corrected too.

rc/test/osdc/object_cacher_stress.cc: fix comparison
Clang complains:
  /home/jenkins/workspace/ceph-freebsd/src/test/osdc/object_cacher_stress.cc:50:26: error: ordered comparison between pointer and zero ('std::atomic<unsigned int> *' and 'int')
      assert(m_outstanding > 0);
               ~~~~~~~~~~~~~ ^ ~
  /home/jenkins/workspace/ceph-freebsd/src/include/assert.h:117:5: note: expanded from macro 'assert'
    ((expr)                                                               \
        ^~~~
Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jun 2, 2017

@joscollin
Ping!

@joscollin

This comment has been minimized.

Member

joscollin commented Jun 2, 2017

@wjwithagen pong.

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jun 2, 2017

@joscollin
What I meant is: Any more things we need to consider?

@joscollin

This comment has been minimized.

Member

joscollin commented Jun 3, 2017

@wjwithagen No, nothing more.

In contrast, I don't feel like removing the count, because it may be implemented by the original author to be useful in the future.

@joscollin joscollin changed the title from test/osdc/object_cacher_stress.cc: fix comparison to test/osdc: fix comparison error and silence warning from -Wunused-value Jun 3, 2017

@joscollin

This comment has been minimized.

Member

joscollin commented Jun 3, 2017

@wjwithagen Looks good. Could you please update the Commit title and the commit message too, so that will be informative about the second line fix ? I have updated the PR Title and PR Description properly.

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jun 3, 2017

@joscollin
Could you than approve it, and set it to needs-qa.
Or submit it?

@joscollin joscollin merged commit 2b56d24 into ceph:master Jun 3, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment