Skip to content
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.

Sign up for GitHub

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

Jump to bottom

1317: Memory leak exposed by address sanitizer in insertable_collection example #1332

Merged
merged 1 commit into from
Mar 23, 2021
Merged

1317: Memory leak exposed by address sanitizer in insertable_collection example #1332

merged 1 commit into from
Mar 23, 2021

Conversation

cz4rs
Copy link
Contributor

@cz4rs cz4rs commented Mar 18, 2021

fixes #1317

@github-actions
Copy link

github-actions bot commented Mar 18, 2021 •
edited
Loading

PR tests (gcc-5, ubuntu, mpich)

Build for f7c5fde

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Mar 18, 2021 •
edited
Loading

PR tests (gcc-6, ubuntu, mpich)

Build for f7c5fde

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Mar 18, 2021 •
edited
Loading

PR tests (clang-3.9, ubuntu, mpich)

Build for f7c5fde

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Mar 18, 2021 •
edited
Loading

PR tests (clang-8, alpine, mpich)

Build for f7c5fde

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Mar 18, 2021 •
edited
Loading

PR tests (gcc-8, ubuntu, mpich, address sanitizer)

Build for f7c5fde

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Mar 18, 2021 •
edited
Loading

PR tests (clang-5.0, ubuntu, mpich)

Build for f7c5fde

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Mar 18, 2021 •
edited
Loading

PR tests (gcc-9, ubuntu, mpich, zoltan)

Build for f7c5fde

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Mar 18, 2021 •
edited
Loading

PR tests (nvidia cuda 10.1, ubuntu, mpich)

Build for f7c5fde

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Mar 18, 2021 •
edited
Loading

PR tests (nvidia cuda 11.0, ubuntu, mpich)

Build for f7c5fde

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Mar 18, 2021 •
edited
Loading

PR tests (gcc-10, ubuntu, openmpi, no LB)

Build for f7c5fde

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Mar 18, 2021 •
edited
Loading

PR tests (intel 18.03, ubuntu, mpich)

Build for f7c5fde

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Mar 18, 2021 •
edited
Loading

PR tests (intel 19, ubuntu, mpich)

Build for f7c5fde

Compilation - successful

Testing - passed

Build log

@codecov
Copy link

codecov bot commented Mar 18, 2021 •
edited
Loading

Codecov Report

Merging #1332 (849e7ea) into develop (b721f2f) will increase coverage by 0.03%.
The diff coverage is n/a.

❗ Current head 849e7ea differs from pull request most recent head f7c5fde. Consider uploading reports for the commit f7c5fde to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1332      +/-   ##
===========================================
+ Coverage    82.25%   82.28%   +0.03%     
===========================================
  Files          734      734              
  Lines        28100    28100              
===========================================
+ Hits         23114    23123       +9     
+ Misses        4986     4977       -9
Impacted Files Coverage Δ
src/vt/vrt/collection/balance/stats_msg.h 100.00% <0.00%> (+2.98%) ⬆️
src/vt/termination/dijkstra-scholten/comm.cc 67.27% <0.00%> (+12.72%) ⬆️

@github-actions
Copy link

github-actions bot commented Mar 18, 2021 •
edited
Loading

PR tests (gcc-7, ubuntu, mpich, trace runtime, LB)

Build for f7c5fde

Compilation - successful

Testing - passed

Build log

@cz4rs cz4rs mentioned this pull request Mar 18, 2021
Closed
@cz4rs cz4rs force-pushed the 1317-insertable-collection-memory-leak branch from 88fb1a4 to b14f8b6 Compare March 18, 2021 19:57
@cz4rs cz4rs marked this pull request as ready for review March 18, 2021 19:57
@cz4rs
Copy link
Contributor Author

cz4rs commented Mar 18, 2021

In the end it's LSAN_OPTIONS - there's a separate variable and slightly different file format for LeakSanitizer.

I have verified that this is enough to silence the error.
Note: if a suppression occurs, it will show up in stdout without failing the build:

-----------------------------------------------------
Suppressions used:
  count      bytes template
      1         64 MPIDI_CH3U_Receive_data_unexpected
-----------------------------------------------------

@cz4rs cz4rs force-pushed the 1317-insertable-collection-memory-leak branch from b14f8b6 to 8350343 Compare March 18, 2021 23:10
@cz4rs cz4rs marked this pull request as draft March 18, 2021 23:45
@cz4rs cz4rs force-pushed the 1317-insertable-collection-memory-leak branch 2 times, most recently from 6c064a4 to 95de953 Compare March 19, 2021 09:27
@cz4rs cz4rs marked this pull request as ready for review March 19, 2021 09:39
lifflander
lifflander requested changes Mar 19, 2021
View reviewed changes
Copy link
Collaborator

@lifflander lifflander left a comment

Choose a reason for hiding this comment

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

Instead of baking this into the dockerfiles (and thus the container images) I think it would be better to make it a variable that can be passed through from docker-compose. What do you think?

@cz4rs
Copy link
Contributor Author

cz4rs commented Mar 19, 2021

Instead of baking this into the dockerfiles (and thus the container images) I think it would be better to make it a variable that can be passed through from docker-compose. What do you think?

Sounds reasonable, it will allow to keep non-asan builds cleaner 👍

@cz4rs cz4rs force-pushed the 1317-insertable-collection-memory-leak branch 3 times, most recently from 1011a1a to 18f2c20 Compare March 19, 2021 22:02
@cz4rs cz4rs force-pushed the 1317-insertable-collection-memory-leak branch from 18f2c20 to f7c5fde Compare March 19, 2021 22:23
cz4rs
cz4rs commented Mar 19, 2021
View reviewed changes
scripts/azure-workflow-template.yml
@@ -44,6 +44,7 @@ variables:
TS_YEAR: 0
TS_MONTH: 0
TS_DAY: 0
[% IF lsan_options %] LSAN_OPTIONS: [% lsan_options %][% END %]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

make sure that there is no trailing whitespace (IF works better here than enabling TRIM option or using [% -%])

@cz4rs
Copy link
Contributor Author

cz4rs commented Mar 19, 2021 •
edited
Loading

@lifflander
It should be good now, except for gcc-7 build reaching it's limits again:

edit: the build has passed after a couple of re-runs

image

@cz4rs cz4rs requested a review from lifflander March 19, 2021 23:54
@lifflander
Copy link
Collaborator

@lifflander
It should be good now, except for gcc-7 build reaching it's limits again:

edit: the build has passed after a couple of re-runs

When limitation was it hitting? Time limit or memory?

lifflander
lifflander approved these changes Mar 22, 2021
View reviewed changes
Copy link
Collaborator

@lifflander lifflander left a comment

Choose a reason for hiding this comment

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

Looks good!

@cz4rs
Copy link
Contributor Author

cz4rs commented Mar 23, 2021

@lifflander
It should be good now, except for gcc-7 build reaching it's limits again:
edit: the build has passed after a couple of re-runs

When limitation was it hitting? Time limit or memory?

It was disk space.

@cz4rs cz4rs merged commit fbd0420 into develop Mar 23, 2021
@lifflander lifflander mentioned this pull request Mar 23, 2021
Open
@cz4rs cz4rs mentioned this pull request Jun 15, 2021
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lifflander lifflander lifflander approved these changes

@JacobDomagala JacobDomagala Awaiting requested review from JacobDomagala

@jstrzebonski jstrzebonski Awaiting requested review from jstrzebonski

@nlslatt nlslatt Awaiting requested review from nlslatt

@nmm0 nmm0 Awaiting requested review from nmm0

@PhilMiller PhilMiller Awaiting requested review from PhilMiller

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Memory leak exposed by address sanitizer in insertable_collection example
2 participants
@cz4rs @lifflander