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

Update temp file handling for windows support #1036

Merged
merged 1 commit into from
Mar 5, 2020

Conversation

isuruf
Copy link
Contributor

@isuruf isuruf commented Feb 27, 2020

No description provided.

@DavidTruby
Copy link
Collaborator

I think we might be better off here to use a std::optional<llvm::sys::fs::file_t> instead of representing non-present values as -1, and try and use only the functions from llvm::sys::fs that operate on file_t instead of int. Then we can avoid using platform specific functions. Would that work?

@isuruf
Copy link
Contributor Author

isuruf commented Feb 28, 2020

That was my first thought, but createUniqueFile takes only a file descriptor (int) and doesn't take in a file_t.

@DavidTruby
Copy link
Collaborator

Ah yes, I remember why I did it this way now!
Perhaps we can call createUniquePath ourselves and use that path to call createNativeFile?

@DavidTruby
Copy link
Collaborator

The best solution is probably to add a createUniquePath function to llvm that returns a file_t instead. I can look in to that.

@isuruf
Copy link
Contributor Author

isuruf commented Feb 28, 2020

Perhaps we can call createUniquePath ourselves and use that path to call createNativeFile?

Race condition?

@isuruf
Copy link
Contributor Author

isuruf commented Feb 29, 2020

Used llvm::sys::fs::convertFDToNativeFile which is found in LLVM 9.0.0 and above.

Copy link
Collaborator

@DavidTruby DavidTruby left a comment

Choose a reason for hiding this comment

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

Perhaps we can call createUniquePath ourselves and use that path to call createNativeFile?

Race condition?

LLVM does exactly this internally in createUniqueFile, so if it would be a race condition here it would be there also. However, your convertFDToNativeFile also works fine.

I'm going to investigate adding a createUniqueNativeFile to LLVM, at which point we should be able to switch over to exclusively using file_t, but in the mean time this fix works fine and we should merge it.

Thanks!

@sscalpone
Copy link
Member

@isuruf Please update the commit message to explain why this change is required. (This explanation is in #1036 (comment) and in your reply.)

Also, please squash into a single commit.

The struct Temp is used in the function call createUniqueFile
which only takes in a file descriptor instead of a file handler.
In Unix these are the same thing, but in Windows they are different.
Therefore, the type of the member of struct Temp is changed
from file handler to file descriptor and when closing the file
the file descriptor is converted to a file handler.
@isuruf
Copy link
Contributor Author

isuruf commented Mar 5, 2020

Done

Copy link
Member

@sscalpone sscalpone left a comment

Choose a reason for hiding this comment

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

@DavidTruby Looking forward to your upstream changes

@sscalpone sscalpone merged commit a8edb32 into flang-compiler:master Mar 5, 2020
@isuruf isuruf deleted the patch-5 branch March 5, 2020 15:12
Sameeranjoshi pushed a commit to Sameeranjoshi/f18 that referenced this pull request Mar 24, 2020
The struct Temp is used in the function call createUniqueFile
which only takes in a file descriptor instead of a file handler.
In Unix these are the same thing, but in Windows they are different.
Therefore, the type of the member of struct Temp is changed
from file handler to file descriptor and when closing the file
the file descriptor is converted to a file handler.
swift-ci pushed a commit to swiftlang/llvm-project that referenced this pull request Apr 9, 2020
The struct Temp is used in the function call createUniqueFile
which only takes in a file descriptor instead of a file handler.
In Unix these are the same thing, but in Windows they are different.
Therefore, the type of the member of struct Temp is changed
from file handler to file descriptor and when closing the file
the file descriptor is converted to a file handler.

Original-commit: flang-compiler/f18@a8edb32
Reviewed-on: flang-compiler/f18#1036
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this pull request Oct 7, 2022
The struct Temp is used in the function call createUniqueFile
which only takes in a file descriptor instead of a file handler.
In Unix these are the same thing, but in Windows they are different.
Therefore, the type of the member of struct Temp is changed
from file handler to file descriptor and when closing the file
the file descriptor is converted to a file handler.

Original-commit: flang-compiler/f18@a8edb32
Reviewed-on: flang-compiler/f18#1036
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants