-
Notifications
You must be signed in to change notification settings - Fork 261
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
nc_test/nc_test fails valgrind #915
Comments
After cleaning up the test a bit, I still have the remaining issues. @DennisHeimbigner these have to do with the rc file. Apparently the resources malloced for it are never freed. Where should they be freed? When the last open file is closed?
|
This same issue is also affecting nc_test, which fails like this:
|
The problem is that there a number of global pieces of data that However... I did define a HIDDEN nc_finallze() function that will free I am still of two minds about adding this function to netcdf.h |
How about we maintain a counter of open files. When count goes from 1 to 0, nc_close() calls nc_finalize(). When count goes from 0 to 1, nc_open/nc_create call nc_initialize()? It would be a bummer if we had to introduced nc_finaize() as a public function. I think we can keep track of when it needs to be called, and spare the user knowledge of it. |
I could see that getting called much too often. There are times when we open multiple (in the hundreds to thousands) of files. At times this needs to be done one-at-a-time so we would be calling nc_initialize() / nc_finalize() hundreds to thousands of times per run which probably isn't optimal just to save a valgrind warning for memory that will be destroyed by the operating system a millisecond later... I think that some libraries register a function with atexit() . The program then calls this function at program exit. Not sure how portable this is, but I have seen it in multiple libraries; I think it is used or was used in HDF5. |
Since atexit() is available under windows and, apparently, OS/X, it is probably |
Valgrind will see it. Valgrind checks after program exit. |
OK, I withdraw my objection to nc_finalize(). What if we make it public and optional. Only users who want to clean up all memory (as for valgrind runs) need bother with it? I will add it to some of the tests that are failing valgrind and see if it sorts them out... |
But hold on. If this is a case of memory that was allocated in nc_initialize(), then how come only a couple of tests are affected? Seems like every test should report this problem, if it is because of normal operation of nc_initialize(). Also, I cleaned up tst_fill_attr_vanish.c a bit, and now I am no longer seeing the problem there. Which is interesteing because I didn't think that anything in my clean-up of the test would fix the leak. I will continue to investigate. |
I don't think that valgrind considers the global data case an |
Also, I am creating a PR to enable using atexit() since it seems to be generally |
@DennisHeimbigner I believe you are correct. Valgrind complains in output about global data, but doesn't fail for it. Here's what is really causing nc_test to fail. Looks like I spoke too soon about not finding memory issues in the classic code. This happens to 64-bit offset format, but not classic:
|
@DennisHeimbigner Have you opened a PR for |
I did not bother with a formal ./configure flag; |
Is anyone still seeing this valgrind failure in nc_test/nc_test? |
@DennisHeimbigner Thanks. On the current master (4a5b15b),
Log attached: |
valgrind builds have been passing in my CI for a while now. I will close this ticket. |
Test nc_test4/tst_fill_attr_vanish fails valgrind like this:
The text was updated successfully, but these errors were encountered: