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.

Sign up for GitHub

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

Jump to bottom

Implement readlink #1564

Merged
merged 11 commits into from
Oct 4, 2020
Merged

Implement readlink #1564

merged 11 commits into from
Oct 4, 2020

Conversation

Aaron1011
Copy link
Member

Due to the truncating behavior of readlink, I was not able to
directly use any of the existing C-cstring helper functions.

RalfJung
RalfJung reviewed Oct 3, 2020
View reviewed changes
src/shims/posix/fs.rs Outdated Show resolved Hide resolved
RalfJung
RalfJung reviewed Oct 3, 2020
View reviewed changes
src/shims/posix/fs.rs Outdated Show resolved Hide resolved
RalfJung
RalfJung reviewed Oct 3, 2020
View reviewed changes
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

Thanks a lot. I particularly like the extensive test. :)

tests/run-pass/fs.rs Outdated Show resolved Hide resolved
tests/run-pass/fs.rs Outdated Show resolved Hide resolved
tests/run-pass/fs.rs Outdated Show resolved Hide resolved
RalfJung
RalfJung reviewed Oct 4, 2020
View reviewed changes
src/shims/os_str.rs Outdated
@@ -62,27 +62,28 @@ fn convert_path_separator<'a>(

impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {}
pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> {

#[cfg(unix)]
fn bytes_to_os_str<'a>(&self, bytes: &'a [u8]) -> InterpResult<'tcx, &'a OsStr> {
Copy link
Member

Choose a reason for hiding this comment

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

Why did you add a self parameter?

Copy link
Member Author

Choose a reason for hiding this comment

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

This makes it easier to invoke from outside this module - if it's an inherent or freestanding method, you need to name this module or this EvalContextExt trait or explicitly.

Copy link
Member

Choose a reason for hiding this comment

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

That's what we do for other context-independent helper functions though, so I think we should also do it here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@RalfJung
Copy link
Member

RalfJung commented Oct 4, 2020

Thanks. :)
@bors r+

@bors
Copy link
Contributor

bors commented Oct 4, 2020

📌 Commit e0ca8c4 has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Oct 4, 2020

⌛ Testing commit e0ca8c4 with merge bba6edf...

bors added a commit that referenced this pull request Oct 4, 2020
@bors
Auto merge of #1564 - Aaron1011:readlink, r=RalfJung
bba6edf 
Implement `readlink`

Due to the truncating behavior of `readlink`, I was not able to
directly use any of the existing C-cstring helper functions.
@bors
Copy link
Contributor

bors commented Oct 4, 2020

💔 Test failed - status-appveyor

RalfJung
RalfJung reviewed Oct 4, 2020
View reviewed changes
tests/run-pass/fs.rs Outdated
// and check that the last byte is not overwritten.
let mut large_buf = vec![0xFF; expected_path.len() + 1];
let res = unsafe { libc::readlink(symlink_c_ptr, large_buf.as_mut_ptr().cast(), large_buf.len()) };
assert_eq!(res, large_buf.len() as isize - 1);
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this assertion is failing on a Windows host when running the Linux target:

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `67`,
 right: `68`', $DIR/fs.rs:242:9

@Aaron1011
Copy link
Member Author

@RalfJung: I've swapped the order of some of the assertions so that the actual paths will be compared first. Can we re-run the tests on windows?

@RalfJung
Copy link
Member

RalfJung commented Oct 4, 2020

Sure: @bors try

You can now also do this yourself: @bors delegate
Just please don't r+ the PR :)

@bors
Copy link
Contributor

bors commented Oct 4, 2020

⌛ Trying commit ec1abfa with merge ddf57d5...

bors added a commit that referenced this pull request Oct 4, 2020
@bors
Auto merge of #1564 - Aaron1011:readlink, r=<try>
ddf57d5 
Implement `readlink`

Due to the truncating behavior of `readlink`, I was not able to
directly use any of the existing C-cstring helper functions.
@RalfJung
Copy link
Member

RalfJung commented Oct 4, 2020 •
edited
Loading

Hm maybe I cannot have multiple bors commands in one comment? Or is the "+" mandatory here?

@bors delegate+

@bors
Copy link
Contributor

bors commented Oct 4, 2020

✌️ @Aaron1011 can now approve this pull request

@bors
Copy link
Contributor

bors commented Oct 4, 2020

💔 Test failed - status-appveyor

@Aaron1011
Copy link
Member Author

Left path: C:\Users\appveyor\AppData\Local\Temp\1\miri_test_fs_link_target.txt
Right path: C:\Users\appveyor\AppData\Local\Temp\1\/miri_test_fs_link_target.txt

It looks like we're joining the path with the wrong platform separator somewhere.

@Aaron1011
Copy link
Member Author

Aaron1011 commented Oct 4, 2020 •
edited
Loading

Oh, I guess that's why those helper functions exist in the first place 😆
I'll add in a truncation flag.

@Aaron1011
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Oct 4, 2020

⌛ Trying commit 9e6320f with merge e202d8c...

bors added a commit that referenced this pull request Oct 4, 2020
@bors
Auto merge of #1564 - Aaron1011:readlink, r=<try>
e202d8c 
Implement `readlink`

Due to the truncating behavior of `readlink`, I was not able to
directly use any of the existing C-cstring helper functions.
RalfJung
RalfJung reviewed Oct 4, 2020
View reviewed changes
src/shims/os_str.rs Outdated
@@ -14,52 +14,11 @@ use rustc_target::abi::LayoutOf;
use crate::*;

/// Represent how path separator conversion should be done.
enum Pathconversion {
pub enum Pathconversion {
Copy link
Member

Choose a reason for hiding this comment

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

While you are at it, could you rename this to PathConversion?

@bors
Copy link
Contributor

bors commented Oct 4, 2020

💔 Test failed - status-appveyor

@Aaron1011
Copy link
Member Author

We're getting closer:

Left: C:/Users/appveyor/AppData/Local/Temp/1/miri_test_fs_link_target.txt
Right: C:\\Users\\appveyor\\AppData\\Local\\Temp\\1\\/miri_test_fs_link_target.txt

We're now returning a 'correct' path from readlink (a C: path, but with / separators). However, we're getting the temporary directory from the MIRI_TEMP, which is set by tests/compiletest.rs outside of the interpreter. I think we'll need to do path conversion when we set MIRI_TEMP, so that we see consistent path separators in an emulated program.

@RalfJung
Copy link
Member

RalfJung commented Oct 4, 2020

Yeah I ran into this before, where paths from the environment are the wrong way...

The thing is, the Linux target should work as well as possible on a Windows host out of the box, so we cannot necessarily expect the environment to do path conversion. But I also see no good way to do this automatically in the interpreter.

We could do something in the test (normalize separators in MIRI_TEMP to the current target)... if we have to adjust the test suite, I think that would be easier than adjusting MIRI_TEMP in tests/compiletest.rs.

@Aaron1011
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Oct 4, 2020

⌛ Trying commit c1c82c2 with merge b53a12b...

bors added a commit that referenced this pull request Oct 4, 2020
@bors
Auto merge of #1564 - Aaron1011:readlink, r=<try>
b53a12b 
Implement `readlink`

Due to the truncating behavior of `readlink`, I was not able to
directly use any of the existing C-cstring helper functions.
@bors
Copy link
Contributor

bors commented Oct 4, 2020

☀️ Try build successful - checks-travis, status-appveyor
Build commit: b53a12b (b53a12bb57ade716de4ce64afddfe511e77c6e31)

RalfJung
RalfJung reviewed Oct 4, 2020
View reviewed changes
tests/run-pass/fs.rs

#[cfg(not(windows))]
return PathBuf::from(tmp.replace("\\", "/"));
}).unwrap_or_else(|_| std::env::temp_dir())
Copy link
Member

Choose a reason for hiding this comment

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

I start to wonder if we should put this in a lazy_static to avoid recomputing all the time.^^ But that is a question for another PR.

RalfJung
RalfJung reviewed Oct 4, 2020
View reviewed changes
src/shims/posix/fs.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

RalfJung commented Oct 4, 2020

Thanks, and nice work. :)
@bors r+

@bors
Copy link
Contributor

bors commented Oct 4, 2020

📌 Commit 3aaab3d has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Oct 4, 2020

⌛ Testing commit 3aaab3d with merge 60c1075...

@bors
Copy link
Contributor

bors commented Oct 4, 2020

☀️ Test successful - checks-travis, status-appveyor
Approved by: RalfJung
Pushing 60c1075 to master...

@bors bors merged commit 60c1075 into rust-lang:master Oct 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Aaron1011 @RalfJung @bors