Replace module writer posix file handling with llvm file handling. by DavidTruby · Pull Request #993 · flang-compiler/f18 (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation25 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

DavidTruby

Note: the file handling interface has changed non-trivially between LLVM 9 and LLVM 10/HEAD. Since we are intending to become a direct part of LLVM, this patch uses the new interface and so introduces a dependency on LLVM HEAD.

@DavidTruby

This doesn't appear to work on the CI, but it works fine for me locally. I'll test it on another machine to see what's going on

@DavidTruby

Should this routine convert from using errno to std::error_code?

Yes, probably. I'll replace that too

@DavidTruby

Seems I was a little hasty here: since we don't rewrite the module unless it's changed, nothing was getting rewritten so with a clean build directory this doesn't work yet.

Although, that does demonstrate that the part where it checks if the module has changed is working fine!

tskeith

Comment on lines 729 to 730

std::size_t result;
if (llvm::sys::fs::file_size(path, result)) {

Choose a reason for hiding this comment

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

This doesn't compile on macos because size_t is unsigned long and the 2nd argument of file_size is uint64_t which is unsigned long long. Declaring result as uint64_t fixes it.

tskeith

Choose a reason for hiding this comment

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

It doesn't appear to avoid writing the .mod file when the contents have changed. This is what I'm seeing:

$ echo "module m; end" > m.f90
$ f18 -fparse-only t.f90
$ stat -f '%Sm' m.mod
Feb 20 11:21:47 2020
$ f18 -fparse-only t.f90
$ stat -f '%Sm' m.mod
Feb 20 11:21:50 2020

The mtime shouldn't have changed.

tskeith

umask(mask);
chmod(tempPath.c_str(), 0666 & ~mask); // temp is created with mode 0600
return Temp{fd, tempPath};
auto prefix = path.substr(0, length - suffix.length());

Choose a reason for hiding this comment

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

Please use braces for initialization.

tskeith

llvm::sys::fs::file_t fd;
llvm::SmallString<16> tempPath;
if (std::error_code err = llvm::sys::fs::createUniqueFile(prefix + "%%%%%%" + suffix, fd, tempPath))
return err;

Choose a reason for hiding this comment

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

Please put braces around the body of the if statement.

Choose a reason for hiding this comment

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

And around the initialization in the if statement.

tskeith

Comment on lines 658 to 660

writer << header;
writer << contents;
writer.flush();

Choose a reason for hiding this comment

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

Can these writes fail? If so the error should be reported.

klausler

std::string buffer(bufSize, '\0');
if (read(fd, &buffer[0], hsize) != static_cast<ssize_t>(hsize) |
std::vector buffer(bufSize, '\0');
if (auto er = fs::readNativeFile(fd, buffer); !er | er.get() != hsize

Choose a reason for hiding this comment

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

braces

tskeith

Comment on lines 681 to 682

| if (auto er = fs::readNativeFile(fd, buffer); !er || er.get() != hsize | | | | ----------------------------------------------------------------------- | - | | std::memcmp(&buffer[0], &header[0], hsize) != 0) { | |

Choose a reason for hiding this comment

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

I think this would be more readable if the declaration of er and the if were separated.

klausler

if (stat(path.c_str(), &statbuf) == 0) {
return static_caststd::size\_t(statbuf.st_size);
static std::uint64_t GetFileSize(const std::string &path) {
std::uint64_t result;

Choose a reason for hiding this comment

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

Should be initialized to forestall warnings.

@DavidTruby

It doesn't appear to avoid writing the .mod file when the contents have changed. This is what I'm seeing:

$ echo "module m; end" > m.f90
$ f18 -fparse-only t.f90
$ stat -f '%Sm' m.mod
Feb 20 11:21:47 2020
$ f18 -fparse-only t.f90
$ stat -f '%Sm' m.mod
Feb 20 11:21:50 2020

The mtime shouldn't have changed.

I've rewritten the FileContentsMatch function, it works correctly for me now. Can you check if it works at your end?

@tskeith

I've rewritten the FileContentsMatch function, it works correctly for me now. Can you check if it works at your end?

Yes, it works for me now. Thanks.

tskeith

@sscalpone

@DavidTruby Would you please add a test that checks that the file modification time is appropriately modified and preserved?

@DavidTruby

Would you please add a test that checks that the file modification time is appropriately modified and preserved?

I'll have to add a flag to the driver and the semantics parser to print info from the module writer so that this info can be checked by lit, as there's no reliable way to get the mtime at a high enough resolution to check it in a test. Is that acceptable?

@tskeith

Would you please add a test that checks that the file modification time is appropriately modified and preserved?

I'll have to add a flag to the driver and the semantics parser to print info from the module writer so that this info can be checked by lit, as there's no reliable way to get the mtime at a high enough resolution to check it in a test. Is that acceptable?

Can you use a python script to perform the test? That seems simpler.

@sscalpone

Would you please add a test that checks that the file modification time is appropriately modified and preserved?

I'll have to add a flag to the driver and the semantics parser to print info from the module writer so that this info can be checked by lit, as there's no reliable way to get the mtime at a high enough resolution to check it in a test. Is that acceptable?

Can you use a python script to perform the test? That seems simpler.

Have a command-line option to display an info msg would be nice and potentially useful in other situations, whatever they may be.

@DavidTruby

Can you use a python script to perform the test? That seems simpler.

I tried this first, but python's getmtime function only has a reported resolution of one second, and it doesn't seem to actually report even with that resolution correctly.

I think the debug information could be useful anyway, so I've added it in. We might want to make the flag a more general debug flag than I've added, I can change the name easily.

tskeith

@@ -0,0 +1,9 @@
! RUN: rm -r %t.dir && mkdir %t.dir && cd %t.dir

Choose a reason for hiding this comment

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

The rm can fail if the directory isn't there, so this should be rm -rf ...

tskeith

end module m
! CHECK_FIRST_WRITE: Processing module {{.*}}.mod: module written
! CHECK_SECOND_WRITE: Processing module {{.*}}.mod: module unchanged, not writing

Choose a reason for hiding this comment

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

A more thorough test would be to compile an initial version of m, then make a change that should result in the same .mod file and verify it is not rewritten. E.g. change:

to:

dimension :: x(10)
public :: x
real :: x

Then make a change that should result in a change (e.g. public to private) and verify it is rewritten.

Choose a reason for hiding this comment

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

Thanks for the suggestion, I've changed the test to do exactly this

tskeith

Choose a reason for hiding this comment

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

Looks good

sscalpone

Choose a reason for hiding this comment

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

Hi @DavidTruby , please resolve the merge conflicts and squash.

@DavidTruby

@sscalpone I've rebased and squash this, it should be ready for merge now.
Thanks!

@DavidTruby

NOTE: This commit introduces a dependency on LLVM HEAD

swift-ci pushed a commit to swiftlang/llvm-project that referenced this pull request

Apr 9, 2020

@DavidTruby

mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this pull request

Oct 7, 2022

@DavidTruby