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

Improve POSIX compliance in CLI/FileUtils.cpp #1064

Merged
merged 4 commits into from
Oct 12, 2023

Conversation

SamuraiCrow
Copy link
Contributor

Some POSIX platforms, such as Haiku and some BSDs, don't supply DT_* identifiers and the corresponding d_type field in stat. This fixes that and has been tested to still work on 64-bit Linux as well as 64-bit Haiku.

@zeux
Copy link
Collaborator

zeux commented Oct 10, 2023

Thanks for the contribution!

I'm not sure what ATTOIF is / where it is supposed to come from, but it is not defined at the point of the check on either macOS or Ubuntu Linux with standard toolchains (clang/gcc respectively). As such, this PR would result in us having to issue an extra syscall for each file or folder met in the traversal, which is probably not intentional.

Copy link
Collaborator

@zeux zeux left a comment

Choose a reason for hiding this comment

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

Needs a different compatibility strategy so that macOS/Linux by default do not need to stat the file.

@SamuraiCrow
Copy link
Contributor Author

@zeux Thanks! I'll try to find where that macro is defined. It's supposed to translate the d_type parameter into the mode_t bitfields. I was so happy to find something that worked on both Haiku and Linux that it didn't occur to me that it might not be defined.

@SamuraiCrow
Copy link
Contributor Author

SamuraiCrow commented Oct 11, 2023

Apple has it defined in the last entry in dirent.h. I'll try checking for an include directive for sys/types.h and dirent.h and see if it helps.

@zeux
Copy link
Collaborator

zeux commented Oct 11, 2023

Apple has it defined in the last entry in dirent.h. I'll try checking for an include directive for sys/types.h and dirent.h and see if it helps.

Ah, maybe that's the issue here - your patch is using ATTOIF instead of DTTOIF?

@SamuraiCrow
Copy link
Contributor Author

There. Sorry about the typos. It should be smaller and less branchy by eliminating the double-pipe or statements (d_type==x || mode & S_x) becomes (mode & S_x) on the if statements.

@SamuraiCrow SamuraiCrow requested a review from zeux October 11, 2023 03:18
@zeux
Copy link
Collaborator

zeux commented Oct 11, 2023

Thanks - this is better, but we're not there yet. I don't think the switch to & is correct. What is it motivated by?

From Linux bits/stat.h:

/* File types.  */
#define __S_IFDIR       0040000 /* Directory.  */
#define __S_IFCHR       0020000 /* Character device.  */
#define __S_IFBLK       0060000 /* Block device.  */
#define __S_IFREG       0100000 /* Regular file.  */
#define __S_IFIFO       0010000 /* FIFO.  */
#define __S_IFLNK       0120000 /* Symbolic link.  */
#define __S_IFSOCK      0140000 /* Socket.  */

A bit test vs IFDIR will also cover block devices. This will cause us to recurse into block devices which will probably not end well.
A bit test vs IFREG also covers IFLNK and IFSOCK. This is particularly a problem for IFLNK that are explicitly ignored here; a link to a folder will now behave differently, and the entire IFLNK branch appears to be dead.

Additionally, because some values (IFLNK in particular) have multiple bits set, testing using & doesn't test the mask correctly.

@SamuraiCrow
Copy link
Contributor Author

OK. This should be better. The double equals checks are back in place.

Copy link
Collaborator

@zeux zeux left a comment

Choose a reason for hiding this comment

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

Thanks! I think this looks good now.

@vegorov-rbx vegorov-rbx merged commit 9843364 into luau-lang:master Oct 12, 2023
6 checks passed
AmaranthineCodices added a commit that referenced this pull request Oct 13, 2023
## What's Changed

- Improve POSIX compliance in `CLI/FileUtils.cpp` by @SamuraiCrow #1064
- `AstStat*::hasEnd` is deprecated; use `AstStatBlock::hasEnd` instead
- Added a lint for common misuses of the `#` operator
- Luau now issues deprecated diagnostics for some uses of `getfenv` and
`setfenv`
- Fixed a case where we included a trailing space in some error
stringifications

### Compiler

- Do not do further analysis in O2 on self functions
- Improve detection of invalid repeat..until expressions vs continue

### New Type Solver

- We now cache subtype test results to improve performance
- Improved operator inference mechanics (aka type families)
- Further work towards type states
- Work towards [new non-strict
mode](https://github.com/Roblox/luau/blob/master/rfcs/new-nonstrict.md)
continues

### Native Codegen

- Instruction last use locations should follow the order in which blocks
are lowered
- Add a bonus assertion to IrLoweringA64::tempAddr

---

Co-authored-by: Arseny Kapoulkine <[email protected]>
Co-authored-by: Vyacheslav Egorov <[email protected]>
Co-authored-by: Andy Friesen <[email protected]>
Co-authored-by: Aaron Weiss <[email protected]>
Co-authored-by: Alexander McCord <[email protected]>
Co-authored-by: Vighnesh Vijay <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants