Cleaning project fails once the binary is expanded in project explorer on Windows

[umairsair]

Steps:

1. Create a managed project and build it
2. Expand the built binary available in binary container in project explorer view
3. Now clean the project, clean will fail irrespective of number of tries you do

Reason:

For finding the sources for binary, Elf instance is created and Section.mapSectionData(..) creates MappedByteBuffer of channel which locks the file on Windows until its garbage collected, see following
https://bugs.java.com/bugdatabase/view_bug.do?bug_id=4715154

Solution:

Made ISymbolReader AutoCloseable and user is responsible to properly close it. In case of dwarf reader, we remove all the references of ByteBuffer and call gc.

---

This solution extends work that was done in Bug 553674 to make more classes AutoCloseable.

There are a couple of things left to do:

• Directly delete the deprecated method
• Document breaking API changes in https://github.com/eclipse-cdt/cdt/blob/main/NewAndNoteworthy/CHANGELOG-API.md
• Document API improvement (using autocloseable) in https://github.com/eclipse-cdt/cdt/blob/main/NewAndNoteworthy/CDT-11.0.md#api-changes-current-and-planned

[github-actions]

Test Results

     578 files       578 suites   45m 34s ⏱️
10 834 tests 10 700 ✔️ 134 💤 0 ❌
10 856 runs  10 722 ✔️ 134 💤 0 ❌

Results for commit dca919c.

♻️ This comment has been updated with latest results.

[jonahgraham]

Please disregard the testModelBuilderBug274490 (org.eclipse.cdt.core.model.tests.CModelBuilderBugsTest) failure. I'll add it to #117, it looks like #128

[jonahgraham]

I don't think overall this is the correct solution. Mapping was explicitly used with the comment "use mapping of sections for dwarf parser rather than allocating memory which avoids out of memory exceptions for large symbol files." in 7ef8d28

Can you comment if that is no longer relevant?

As for the the garbage collection, I think that is why 7ef8d28 calls System.gc in dispose method.

[umairsair]

I don't think overall this is the correct solution. Mapping was explicitly used with the comment "use mapping of sections for dwarf parser rather than allocating memory which avoids out of memory exceptions for large symbol files." in 7ef8d28

Can you comment if that is no longer relevant?

I think that both current and previous solution are same as both eventually gives the DirectByteBuffer.. maybe there is something deeper in the DirectByteBuffer implementation for both cases which I am missing? Apparently I see no issue..

As for the the garbage collection, I think that is why 7ef8d28 calls System.gc in dispose method.

IMO this logic is not valid for all scenarios.. it is valid when its only called form finalize.. but its called from close as well as by the user which essentially doesn't mean that all the references of it are gone including the ByteBuffer instance references, so this gc doesn't work for scenarios where dispose is called from close or by user..

[jonahgraham]

I don't think overall this is the correct solution. Mapping was explicitly used with the comment "use mapping of sections for dwarf parser rather than allocating memory which avoids out of memory exceptions for large symbol files." in 7ef8d28
Can you comment if that is no longer relevant?

I think that both current and previous solution are same as both eventually gives the DirectByteBuffer.. maybe there is something deeper in the DirectByteBuffer implementation for both cases which I am missing? Apparently I see no issue..

Try a big file, you can map it without running out of memory, but you can't make a large ByteBuffer.

Here is a simple example:

• Create a large file (> Xmx size provided later)
• Use a program like this:

package org.eclipse.cdt.utils.elf;

import java.io.IOException;
import java.nio.ByteBuffer;
import java.nio.channels.FileChannel.MapMode;

import org.eclipse.cdt.utils.ERandomAccessFile;

public class Main {
	public static void main(String[] args) throws IOException {
		System.out.println("Opening " + args[0]);
		ERandomAccessFile efile = new ERandomAccessFile(args[0], "r"); //$NON-NLS-1$
		long size = efile.getChannel().size();
		long sizeToRead = Math.min(Integer.MAX_VALUE / 2, size);
		ByteBuffer byteBuffer = efile.getChannel().map(MapMode.READ_ONLY, 0, sizeToRead).load().asReadOnlyBuffer();
		System.out.println("Type of byteBuffer " + byteBuffer.getClass());
		// This line will fail:
		ByteBuffer buff = ByteBuffer.allocateDirect((int) sizeToRead);
		System.out.println("Type of byteBuffer " + byteBuffer.getClass());
	}
}

Run it with an -Xmx smaller than sizeToRead and you will see the allocateDirect will fail with:

Exception in thread "main" java.lang.OutOfMemoryError: Cannot reserve 1073741823 bytes of direct buffer memory (allocated: 0, limit: 536870912)

As for the the garbage collection, I think that is why 7ef8d28 calls System.gc in dispose method.

IMO this logic is not valid for all scenarios.. it is valid when its only called form finalize.. but its called from close as well as by the user which essentially doesn't mean that all the references of it are gone including the ByteBuffer instance references, so this gc doesn't work for scenarios where dispose is called from close or by user..

I agree - and this needs to be fixed, but not by allocating everything into memory. It may take some more judicious use of allocated memory so the handle can be freed and mapped buffers where that is needed.

The other alternative is to fix the whole lifecycle so that the useful stuff is loaded out of the file, then the file is closed. The actually needed data is going to be much smaller than the whole section needing to be in memory.

Because this release is a new major release it is a good opportunity to make such drastic changes.

[umairsair]

Thanks for the guidance @jonahgraham

I have added a commit where I have made ISymbolReader AutoCloseable and user is responsible for closing it properly.

And if user calls mapSectionData, it'll be his responsibility to make sure that after using ByteBuffer he properly un-reference it so that its garbage collected.. let me know your feedback..

[jonahgraham]

@umairsair umairsair requested a review from jonahgraham 1 hour ago

@umairsair I'll try to review properly soon, on first pass this looks correct.

[jonahgraham]

This looks fine, thanks for the change.

I am going to merge #135 first so we don't spend even more time reviewing code in deprecated classes.

(moved TODO list so it shows up in GitHub UI better)

[jonahgraham]

#135 was taking too long to resolve, so I have rebased this change, added the N&N entry and once I get a clean build I will merge it.

[jonahgraham]

Thank you @umairsair for the improvement.