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

Add tests for deallocation bug, and a workaround #3801

Merged
merged 4 commits into from
Apr 7, 2024
Merged

Conversation

certik
Copy link
Contributor

@certik certik commented Apr 6, 2024

The workaround leaks memory, so I think we should not commit this, and rather implement a proper fix.

@certik
Copy link
Contributor Author

certik commented Apr 7, 2024

I think the dftatom fails because we are leaking to much memory and run out of memory.

@certik certik added the stdlib Issues related to compiling fortran-lang/stdlib label Apr 7, 2024
@certik certik marked this pull request as ready for review April 7, 2024 14:54
@certik
Copy link
Contributor Author

certik commented Apr 7, 2024

This PR allows me to compile and run stdlib without any segfaults. There are two tests added, but only one enabled for LFortran, and it tests the issue I had with stdlib.

In order to release the blog post, we need stdlib to work on all platforms, and this PR is one way to achieve it.

An alternative is to figure out a workaround in the Fortran source code of stdlib, but I wasn't able to figure out how to avoid it.

The second alternative is to wait until we have a proper fix for strings, which @Shaikh-Ubaid is working on, but that might take a while.

So I think it's fine to merge this PR, release stdlib blog post, and then work on a proper fix for strings.

@Shaikh-Ubaid, @Pranavchiku, @czgdp1807, let me know what you think.

@certik certik mentioned this pull request Apr 7, 2024
6 tasks
@Shaikh-Ubaid
Copy link
Member

It seems fine to me.

@Pranavchiku
Copy link
Contributor

Pranavchiku commented Apr 7, 2024

Can you try this diff,

diff --git a/subprojects/test-drive/src/testdrive.F90 b/subprojects/test-drive/src/testdrive.F90
index 1332be3..420d35f 100644
--- a/subprojects/test-drive/src/testdrive.F90
+++ b/subprojects/test-drive/src/testdrive.F90
@@ -405,6 +405,7 @@ contains
     !$omp critical(testdrive_testsuite)
     write(unit, '(a)') message
     !$omp end critical(testdrive_testsuite)
+    allocate(error)
     if (allocated(error)) then
       call clear_error(error)
     end if

@Pranavchiku
Copy link
Contributor

Fine with me as well, we can go ahead and release blog.

Copy link
Contributor

@Pranavchiku Pranavchiku left a comment

Choose a reason for hiding this comment

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

Works for now, releasing blog is fine with me considering the failure not in Linux, latest macOS versions.

@certik
Copy link
Contributor Author

certik commented Apr 7, 2024

@Pranavchiku I tried applying the following patch to this PR:

$ git diff bug4
diff --git a/integration_tests/dealloc_01.f90 b/integration_tests/dealloc_01.f90
index ebf759fcd..a9dad9ff4 100644
--- a/integration_tests/dealloc_01.f90
+++ b/integration_tests/dealloc_01.f90
@@ -72,6 +72,7 @@ contains
     end if
     call make_output(message, test_var, error)
     write(unit, '(a)') message
+    allocate(error)
   end subroutine run_unittest
 
   pure function test_skipped(error) result(is_skipped)
diff --git a/src/libasr/runtime/lfortran_intrinsics.c b/src/libasr/runtime/lfortran_intrinsics.c
index 9257dec49..bf69bc55a 100644
--- a/src/libasr/runtime/lfortran_intrinsics.c
+++ b/src/libasr/runtime/lfortran_intrinsics.c
@@ -1407,9 +1407,7 @@ LFORTRAN_API void _lfortran_strcpy(char** x, char *y, int8_t free_target)
 {
     if (free_target) {
         if (*x) {
-            // We should free `x` here, but cannot due to:
-            // https://github.com/lfortran/lfortran/issues/3787
-            //free((void *)*x);
+            free((void *)*x);
         }
         *x = (char*) malloc((strlen(y) + 1) * sizeof(char));
         _lfortran_string_init(strlen(y) + 1, *x);

But the error is still there.

@certik certik merged commit f8ead00 into lfortran:main Apr 7, 2024
21 checks passed
@certik certik deleted the bug4 branch April 7, 2024 16:00
@certik
Copy link
Contributor Author

certik commented Apr 7, 2024

I merged it. The workaround is simple to revert, but let's see if we can release the stdlib blog post now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Issues related to compiling fortran-lang/stdlib tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants