chapel-lang / chapel

a Productive Parallel Programming Language
https://chapel-lang.org
Other
1.8k stars 421 forks source link

problems with memory management of barriers #15116

Open mppf opened 4 years ago

mppf commented 4 years ago

After PR #15041, there are failures in test/parallel/taskPar/elliot/barrier/array-of-barriers.chpl due to errors in the memory management of barriers. Memory management for barriers started in PR #4945.

Here is an example that changed behavior after PR #15041:

var barArr: [1..1] Barrier;
barArr[1] = new Barrier(1);

the new barrier is now deinited immediately after the assignment statement. The array stores a Barrier with isowned=false, which is then used in a use-after-free.

Even before that PR, the memory management strategy in Barrier (always set isowned=true in = or init=) was problematic. In particular, it is impossible to initialize some Barrier objects in a loop. For example:

use Barriers;
var ba = [i in 1..10] new Barrier(1);
for bar in ba do bar.barrier();

has use-after-free memory errors even before my PR.

How can Barrier manage memory more appropriately? (And, should Barrier be renamed to barrier ?)

Here are some possible short-term directions:

bradcray commented 4 years ago

Is this issue expected to have an impact on users? Only if they use arrays of barriers or in general? Does it represent a backslide from the previous release? Should we be prioritizing addressing it?

mppf commented 4 years ago

Is this issue expected to have an impact on users? Only if they use arrays of barriers or in general? Does it represent a backslide from the previous release? Should we be prioritizing addressing it?

I don't expect users have arrays of barriers.

Does it represent a backslide from the previous release?

Arrays created in any kind of loop wouldn't have worked in previous releases.

Should we be prioritizing addressing it?

I suspect arrays of barriers are rare and the typical use case has the barrier in a variable. So I think we could choose to put it off if we like.

However I absolutely think it needs to be fixed.