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.

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

Setting LG_init_has_been_called = false after LAGraph_finalize #184

Open
jamesETsmith opened this issue Apr 5, 2023 · 1 comment
Open

Comments

@jamesETsmith
Copy link

General

I recently came across a problem where LG_init_has_been_called (defined as a global variable in src/utility/LAGr_init.c) isn't set to false in between subtests in test_BreadthFirstSearch which triggers a cascade of problems which were somewhat tough to track down.

I can only reproduce this error with a modified version of clang 6.0.0 where LAGraph is built with LAGRAPH_VANILLA=1 and linked to Lucata's implementation of GraphBLAS).

Details

This is somewhat convoluted to describe so please ask questions if the following isn't clear. When LAGr_init() fails, it calls GrB_finalize, but since the error in LAGraph_Init() isn't caught inside setup() (see here), the subtest proceeds. It will then try to call GrB_finalize() again (if it doesn't crash earlier) which leads to freeing already freed memory.

Proposed Solution

  • Primary Change: add extern bool LG_init_has_been_called; to LAGraph_Finalize.c and set it to false when LAGraph_Finalize() completes successfully
  • Minor change: Wrap LAGraph_Init and LAGraph_Finalize in checks to make sure the user knows if they fail.

I'm happy to open a PR with these changes, but I wanted to discuss them first. Let me know what you think.

@DrTimothyAldenDavis
Copy link
Member

I talked this over with the LAGraph group. We have a different proposed solution.

First of all, I moved the setting of this flag (now called LG_LAGr_Init_has_been_called) to occur at the very end of LAGr_init. This will help in the rare but possible user situation where LAGr_Init fails.

I also added two functions to set/get this flag (see LAGr_Init.c in the v1.1_branch). In normal usage, the flag should only be set by LAGr_Init. It should not be cleared by LAGraph_Finalize, as you suggest, however.

But what you can do is what we have in test_Xinit. That method adds the prototypes of these two set/get functions, so it can clear and set the flag as needed for its tests. In particular, the test_Xinit_brutal method tests LAGr_Init many times, all with pretend failures of running out of memory, and successfully recovers.

LAGraph_Finalize can safely free any partially allocated blocks of memory that were allocated by LAGr_Init.

So if you'd like control over this flag in your tests, just add the prototypes to your tests:

LAGRAPH_PUBLIC void LG_set_LAGr_Init_has_been_called (bool setting) ;
LAGRAPH_PUBLIC bool LG_get_LAGr_Init_has_been_called (void) ;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants