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

Use FILE_ATTRIBUTE_TAG_INFO to get reparse tag #101260

Merged
merged 1 commit into from
Sep 2, 2022

Conversation

ChrisDenton
Copy link
Member

I've been looking at this code recently and it just occurred to me we don't actually use the full reparse data at this point, only the tag. GetFileInformationByHandleEx can do exactly that by filling a FILE_ATTRIBUTE_TAG_INFO struct.

r? @thomcc since you've made changes here recently (which is why I have this code on my mind atm)

This avoid unnecessarily getting the full reparse data when all we need is the tag.
@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 1, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 1, 2022
@ChrisDenton ChrisDenton added the O-windows Operating system: Windows label Sep 1, 2022
Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

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

Ah, great!

self.handle.as_raw_handle(),
c::FileAttributeTagInfo,
ptr::addr_of_mut!(attr_tag).cast(),
mem::size_of::<c::FILE_ATTRIBUTE_TAG_INFO>().try_into().unwrap(),
Copy link
Member

Choose a reason for hiding this comment

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

Kind of goofy to do this here, but I guess I'm not opposed since the unwrap should vanish and I would generally like us to be more careful about a usize->dword.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. I was thinking of our conversation here and noticed it codegened fine. But it is weird. I do think it'd ultimately be better to have a proper wrapper for these cases that can statically assert the size fits into a u32. And also something for the cases that could possibly fail.

@thomcc
Copy link
Member

thomcc commented Sep 1, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Sep 1, 2022

📌 Commit 630f831 has been approved by thomcc

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 1, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 2, 2022
Use `FILE_ATTRIBUTE_TAG_INFO` to get reparse tag

I've been looking at this code recently and it just occurred to me we don't actually use the full reparse data at this point, only the tag. [`GetFileInformationByHandleEx`](https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getfileinformationbyhandleex) can do exactly that by filling a [`FILE_ATTRIBUTE_TAG_INFO`](https://docs.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_attribute_tag_info) struct.

r? `@thomcc` since you've made changes here recently (which is why I have this code on my mind atm)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 2, 2022
Use `FILE_ATTRIBUTE_TAG_INFO` to get reparse tag

I've been looking at this code recently and it just occurred to me we don't actually use the full reparse data at this point, only the tag. [`GetFileInformationByHandleEx`](https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getfileinformationbyhandleex) can do exactly that by filling a [`FILE_ATTRIBUTE_TAG_INFO`](https://docs.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_attribute_tag_info) struct.

r? ``@thomcc`` since you've made changes here recently (which is why I have this code on my mind atm)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 2, 2022
Use `FILE_ATTRIBUTE_TAG_INFO` to get reparse tag

I've been looking at this code recently and it just occurred to me we don't actually use the full reparse data at this point, only the tag. [`GetFileInformationByHandleEx`](https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getfileinformationbyhandleex) can do exactly that by filling a [`FILE_ATTRIBUTE_TAG_INFO`](https://docs.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_attribute_tag_info) struct.

r? ```@thomcc``` since you've made changes here recently (which is why I have this code on my mind atm)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 2, 2022
Use `FILE_ATTRIBUTE_TAG_INFO` to get reparse tag

I've been looking at this code recently and it just occurred to me we don't actually use the full reparse data at this point, only the tag. [`GetFileInformationByHandleEx`](https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getfileinformationbyhandleex) can do exactly that by filling a [`FILE_ATTRIBUTE_TAG_INFO`](https://docs.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_attribute_tag_info) struct.

r? ````@thomcc```` since you've made changes here recently (which is why I have this code on my mind atm)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 2, 2022
Use `FILE_ATTRIBUTE_TAG_INFO` to get reparse tag

I've been looking at this code recently and it just occurred to me we don't actually use the full reparse data at this point, only the tag. [`GetFileInformationByHandleEx`](https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getfileinformationbyhandleex) can do exactly that by filling a [`FILE_ATTRIBUTE_TAG_INFO`](https://docs.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_attribute_tag_info) struct.

r? `````@thomcc````` since you've made changes here recently (which is why I have this code on my mind atm)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 2, 2022
Use `FILE_ATTRIBUTE_TAG_INFO` to get reparse tag

I've been looking at this code recently and it just occurred to me we don't actually use the full reparse data at this point, only the tag. [`GetFileInformationByHandleEx`](https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getfileinformationbyhandleex) can do exactly that by filling a [`FILE_ATTRIBUTE_TAG_INFO`](https://docs.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_attribute_tag_info) struct.

r? ``````@thomcc`````` since you've made changes here recently (which is why I have this code on my mind atm)
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 2, 2022
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#100121 (Try normalizing types without RevealAll in ParamEnv in MIR validation)
 - rust-lang#100200 (Change implementation of `-Z gcc-ld` and `lld-wrapper` again)
 - rust-lang#100814 ( Porting 'compiler/rustc_trait_selection' to translatable diagnostics - Part 1)
 - rust-lang#101215 (Also replace the version placeholder in rustc_attr)
 - rust-lang#101260 (Use `FILE_ATTRIBUTE_TAG_INFO` to get reparse tag)
 - rust-lang#101323 (Remove unused .toggle-label CSS rule)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1e008fe into rust-lang:master Sep 2, 2022
@rustbot rustbot added this to the 1.65.0 milestone Sep 2, 2022
@RalfJung
Copy link
Member

RalfJung commented Sep 2, 2022

Is it possible that this caused #101344 ?

@ChrisDenton
Copy link
Member Author

I admit I'm struggling to see how it would be.

@thomcc
Copy link
Member

thomcc commented Sep 3, 2022

Given that its an ICE which contains:

query stack during panic:
#0 [mir_drops_elaborated_and_const_checked] elaborating drops for `sys::windows::fs::<impl at D:\a\rust\rust\library\std\src\sys\windows\fs.rs:278:1: 278:10>::file_attr`
#1 [optimized_mir] optimizing MIR for `sys::windows::fs::<impl at D:\a\rust\rust\library\std\src\sys\windows\fs.rs:278:1: 278:10>::file_attr`
end of query stack

it seems like this did cause the issue, since compiling this code seems to have unearthed a latent compiler bug which results in this ICE.

Unless the code is actually wrong I don't think we want to back it out, but if it's impossible to fix in rustc and is a huge issue for miri I suppose we could tweak it or keep the old version behind #[cfg(miri)]. I'd rather not, tho.

@ChrisDenton
Copy link
Member Author

ChrisDenton commented Sep 3, 2022

For sure, I'm not at all saying this didn't trigger a bug. But, as far as I can tell (tho I may be mistaken), this isn't the root cause. And it feels a bit unsatisfactory to fix a symptom and not the root cause. Although I guess a temporary workaround is fine (so long as it is actually temporary).

Also, yeah, I'd really love to avoid #[cfg(miri)] as much as we can get away with.

@ChrisDenton ChrisDenton deleted the attribute-tag branch September 3, 2022 06:44
@RalfJung
Copy link
Member

RalfJung commented Sep 3, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-windows Operating system: Windows S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants