search

flang-compiler / f18

F18 is a front-end for Fortran intended to replace the existing front-end in the Flang compiler
230 stars 48 forks source link

Replace manual mmap with llvm::MemoryBuffer #1032

Closed DavidTruby closed 4 years ago

DavidTruby commented 4 years ago

Fixes #840

DavidTruby commented 4 years ago

@sscalpone could you check if this has the defective fd behaviour you mentioned on the mailing list? I don't believe it should as llvm::MemoryBuffer doesn't keep the fd around after it has mmapped a file.

klausler commented 4 years ago

@sscalpone could you check if this has the defective fd behaviour you mentioned on the mailing list? I don't believe it should as llvm::MemoryBuffer doesn't keep the fd around after it has mmapped a file.

If the open file descriptor is closed, then the file is not mapped into the virtual address space.

DavidTruby commented 4 years ago

If the open file descriptor is closed, then the file is not mapped into the virtual address space.

Per the POSIX standard, this isn't the case. See https://pubs.opengroup.org/onlinepubs/7908799/xsh/mmap.html: " The mmap() function adds an extra reference to the file associated with the file descriptor fildes which is not removed by a subsequent close() on that file descriptor. This reference is removed when there are no more mappings to the file. "

klausler commented 4 years ago

If the open file descriptor is closed, then the file is not mapped into the virtual address space.

Per the POSIX standard, this isn't the case. See https://pubs.opengroup.org/onlinepubs/7908799/xsh/mmap.html: " The mmap() function adds an extra reference to the file associated with the file descriptor fildes which is not removed by a subsequent close() on that file descriptor. This reference is removed when there are no more mappings to the file. "

So the limit on the number of simultaneous open file descriptors won't be a problem? That's my only concern here. Have you tested this with a large number of INCLUDE files?

DavidTruby commented 4 years ago

So the limit on the number of simultaneous open file descriptors won't be a problem? That's my only concern here. Have you tested this with a large number of INCLUDE files?

It shouldn't be a problem, as we shouldn't have that many fds open at any one time. I don't have a test case that would have enough files to reach the fd limit though.

Clang and Swift both use this MemoryBuffer class for their source file managing so I assume this has already been considered. We should check it though, I'll see if I can randomly generate a case.

DavidTruby commented 4 years ago

We should check it though, I'll see if I can randomly generate a case.

I've tried it with a file that INCLUDEs 10,000 other files, which is well over my fd limit per process on my system. I don't get any errors and get correct output from -fget-symbols-sources.

DavidTruby commented 4 years ago

@sscalpone @tskeith is this ready to merge now?

DavidTruby commented 4 years ago

@sscalpone rebased on top of the LLVM streams patch

sscalpone commented 4 years ago

@DavidTruby This PR causes a fatal check when compiling Nyx/Exec/Scaling in my sandbox. I don't know if the problem is with the PR or if the PR is exposing a different issue.

fatal internal error: CHECK(range_.Contains(at)) failed at /home/sjs/work/pgi/f18/pr1032/f18/lib/Parser/provenance.cpp(389)

I can't share the Nyx source tree that I'm using; however, I think it is based on this: https://github.com/AMReX-Astro/Nyx/tree/master/Exec/Scaling.

The failing file is zero length. 

% pwd
.../pr1032/build
% touch zero.f90
% ls -al zero.f90
-rw-rw-r-- 1 xxx yy 0 Mar 22 08:46 zero.f90
% tools/f18/bin/f18 zero.f90

fatal internal error: CHECK(range_.Contains(at)) failed at /home/sjs/work/pgi/f18/pr1032/f18/lib/Parser/provenance.cpp(389)
Aborted
DavidTruby commented 4 years ago

@DavidTruby This PR causes a fatal check when compiling Nyx/Exec/Scaling in my sandbox. I don't know if the problem is with the PR or if the PR is exposing a different issue.

fatal internal error: CHECK(range_.Contains(at)) failed at /home/sjs/work/pgi/f18/pr1032/f18/lib/Parser/provenance.cpp(389)

I can't share the Nyx source tree that I'm using; however, I think it is based on this: https://github.com/AMReX-Astro/Nyx/tree/master/Exec/Scaling.

The failing file is zero length.

% pwd
.../pr1032/build
% touch zero.f90
% ls -al zero.f90
-rw-rw-r-- 1 xxx yy 0 Mar 22 08:46 zero.f90
% tools/f18/bin/f18 zero.f90

fatal internal error: CHECK(range_.Contains(at)) failed at /home/sjs/work/pgi/f18/pr1032/f18/lib/Parser/provenance.cpp(389)
Aborted

@sscalpone I seem to be able to reproduce this with empty files. I'll fix it and add a lit test for empty files.

DavidTruby commented 4 years ago

@sscalpone Should be fixed now. I've left it as a separate commit so you can review the fix, please let me know if it's ok to squash.

DavidTruby commented 4 years ago

@sscalpone I've squashed this to one commit and written a more descriptive commit message