Skip to content

qa: use normal build for valgrind#39561

Merged
liewegas merged 2 commits intoceph:masterfrom
liewegas:valgrind-soname
Feb 19, 2021
Merged

qa: use normal build for valgrind#39561
liewegas merged 2 commits intoceph:masterfrom
liewegas:valgrind-soname

Conversation

@liewegas
Copy link
Member

@liewegas liewegas commented Feb 18, 2021

With some extra valgrind args we can use the tcmalloc-linked binaries! The tradeoff is we have to whitelist any memcheck errors with free(), since tcmalloc causes the delete vs delete[] error due to some internal optimizations (see gperftools/gperftools#792).

Depends on ceph/teuthology#1618

teuthology now knows how to run valgrind against a tcmalloc binary

Signed-off-by: Sage Weil <sage@newdream.net>
This is apparently not going to get fixed any time soon.

gperftools/gperftools#792

Signed-off-by: Sage Weil <sage@newdream.net>
@liewegas liewegas requested a review from markhpc February 18, 2021 20:41
Copy link
Member

@jdurgin jdurgin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since the leaks are still detected according the the valgrind-leaks suite, looks good!

Copy link

@dillaman dillaman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liewegas liewegas merged commit 568eb9e into ceph:master Feb 19, 2021
@liewegas liewegas deleted the valgrind-soname branch February 19, 2021 21:29
djgalloway pushed a commit to ceph/ceph-build that referenced this pull request Feb 24, 2021
We no longer need notmcalloc packages.  Valgrind works now.

See ceph/ceph#39561

Signed-off-by: David Galloway <dgallowa@redhat.com>
Memcheck:Free
fun:free
...
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this MismatchedFree report looks new in the rgw suite:

<error>
  <unique>0xeef7eb</unique>
  <tid>1</tid>
  <kind>MismatchedFree</kind>
  <what>Mismatched free() / delete / delete []</what>
  <stack>
    <frame>
      <ip>0x4C32EA0</ip>
      <obj>/usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so</obj>
      <fn>operator delete[](void*, unsigned long)</fn>
      <dir>/builddir/build/BUILD/valgrind-3.16.0/coregrind/m_replacemalloc</dir>
      <file>vg_replace_malloc.c</file>
      <line>660</line>
    </frame>
    <frame>
      <ip>0x5FBEB86</ip>
      <obj>/usr/lib64/librados.so.2.0.0</obj>
    </frame>
    <frame>
      <ip>0x5FBEF2C</ip>
      <obj>/usr/lib64/librados.so.2.0.0</obj>
    </frame>
    <frame>
      <ip>0x5F7F7C5</ip>
      <obj>/usr/lib64/librados.so.2.0.0</obj>
      <fn>librados::v14_2_0::Rados::shutdown()</fn>
    </frame>

are we missing something in the suppression, or is this unrelated to tcmalloc?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, not quite sure why it's different, but i only tested the new suppression+ normal build with rados/verify --filter valgrind and rados/valgrind-leaks (which ensure we correctly detect a deliberate leak). those test cases don't use librados. also.. librados wouldn't use tcmalloc unless the application using it uses tcmalloc. maybe that changes the path somehow?

in any case, it should be easy to test this locally and see if it is related to -DALLOCATOR=libc vs tcmalloc and, if so, add another suppression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants