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

Use a hardcoded constant instead of calling OpenProcessToken. #119032

Merged
merged 2 commits into from
Feb 17, 2024
Merged

Use a hardcoded constant instead of calling OpenProcessToken. #119032

merged 2 commits into from
Feb 17, 2024

Conversation

smmalis37
Copy link
Contributor

Now that Win 7 support is dropped, we can resurrect #90144.

GetCurrentProcessToken is defined in processthreadsapi.h as:

FORCEINLINE
HANDLE
GetCurrentProcessToken (
VOID
)
{
return (HANDLE)(LONG_PTR) -4;
}

Since it's very unlikely that this constant will ever change, let's just use it instead of making calls to get the same information.

@rustbot
Copy link
Collaborator

rustbot commented Dec 17, 2023

r? @thomcc

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added O-windows Operating system: Windows S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 17, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
deltragon
deltragon suggested changes Dec 17, 2023
View reviewed changes
library/std/src/sys/windows/os.rs Outdated Show resolved Hide resolved
@deltragon
Copy link
Contributor

Additionally, windows 7 was re-added as a tier 3 target in #118150, so it might be good to test with that/keep the old code under a cfg for that target.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@smmalis37
Copy link
Contributor Author

Hmm, doesn't seem to like my cfg for win7 support. What should it be?

@ChrisDenton
Copy link
Member

The target is probably too new to be in the bootstrap compiler (which is currently beta 2013-11-13). The bootstrap compiler will next be updated in a week or two. In the meantime, you can put something like this at the top of the file:

#![cfg_attr(bootstrap, allow(unexpected_cfgs))]

This will tell the bootstrap (stage0) compiler to ignore the error while not affecting the newly built (stage1+) compiler.

@ChrisDenton ChrisDenton assigned ChrisDenton and unassigned thomcc Dec 18, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@ChrisDenton
Copy link
Member

Ok, this looks good. Could you squish your commits in to one?

@Urgau
Copy link
Member

Urgau commented Jan 3, 2024

The target is probably too new to be in the bootstrap compiler (which is currently beta 2013-11-13). The bootstrap compiler will next be updated in a week or two. In the meantime, you can put something like this at the top of the file:

#![cfg_attr(bootstrap, allow(unexpected_cfgs))]

Note that the better and recommended way is to add the exception to the "extra check-cfg list" in bootstrap:

rust/src/bootstrap/src/lib.rs

Lines 91 to 93 in e51e98d

(Some(Mode::Std), "target_env", Some(&["libnx"])),
// (Some(Mode::Std), "target_os", Some(&[])),
(Some(Mode::Std), "target_arch", Some(&["spirv", "nvptx", "xtensa"])),

@bors
Copy link
Contributor

bors commented Jan 13, 2024

☔ The latest upstream changes (presumably #117285) made this pull request unmergeable. Please resolve the merge conflicts.

@ChrisDenton
Copy link
Member

Hi, very sorry for the delay here. This was right around when i was going on holiday and evidently I missed that I hadn't approved it.

Could you rebase on master? The file has moved to library/std/src/sys/pal/windows/os.rs. Also the cfg_attr(bootstrap... line is no longer needed.

@smmalis37
Copy link
Contributor Author

Yeah, I should be able to get back to this next weekend.

@smmalis37
Use a hardcoded constant instead of calling OpenProcessToken.
4071938 
Now that Win 7 support is dropped, we can resurrect rust-lang#90144.

GetCurrentProcessToken is defined in processthreadsapi.h as:

FORCEINLINE
HANDLE
GetCurrentProcessToken (
    VOID
    )
{
    return (HANDLE)(LONG_PTR) -4;
}

Since it's very unlikely that this constant will ever change, let's just use it instead of making calls to get the same information.
@smmalis37
Remove cfg_attr
3b63ede
@smmalis37
Copy link
Contributor Author

Rebase came out clean, think this is ready.

@ChrisDenton
Copy link
Member

Thanks for sticking with this and sorry again for the delay.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 17, 2024

📌 Commit 3b63ede has been approved by ChrisDenton

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 Feb 17, 2024
Nadrieril added a commit to Nadrieril/rust that referenced this pull request Feb 17, 2024
@Nadrieril
Rollup merge of rust-lang#119032 - smmalis37:patch-1, r=ChrisDenton
53239d8 
Use a hardcoded constant instead of calling OpenProcessToken.

Now that Win 7 support is dropped, we can resurrect rust-lang#90144.

GetCurrentProcessToken is defined in processthreadsapi.h as:

FORCEINLINE
HANDLE
GetCurrentProcessToken (
    VOID
    )
{
    return (HANDLE)(LONG_PTR) -4;
}

Since it's very unlikely that this constant will ever change, let's just use it instead of making calls to get the same information.
This was referenced Feb 17, 2024
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 17, 2024
@bors
Auto merge of rust-lang#121227 - Nadrieril:rollup-n6qky3z, r=Nadrieril
ba824a2 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#119032 (Use a hardcoded constant instead of calling OpenProcessToken.)
 - rust-lang#120932 (const_mut_refs: allow mutable pointers to statics)
 - rust-lang#121059 (Add and use a simple extension trait derive macro in the compiler)
 - rust-lang#121135 (coverage: Discard spans that fill the entire function body)
 - rust-lang#121187 (Add examples to document the return type of quickselect functions)
 - rust-lang#121191 (Add myself to review rotation (and a rustbot ping))
 - rust-lang#121192 (Give some intrinsics fallback bodies)
 - rust-lang#121197 (Ensure `./configure` works when `configure.py` path contains spaces)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5f21609 into rust-lang:master Feb 17, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 17, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 17, 2024
@rust-timer
Unrolled build for rust-lang#119032
00c0aef 
Rollup merge of rust-lang#119032 - smmalis37:patch-1, r=ChrisDenton

Use a hardcoded constant instead of calling OpenProcessToken.

Now that Win 7 support is dropped, we can resurrect rust-lang#90144.

GetCurrentProcessToken is defined in processthreadsapi.h as:

FORCEINLINE
HANDLE
GetCurrentProcessToken (
    VOID
    )
{
    return (HANDLE)(LONG_PTR) -4;
}

Since it's very unlikely that this constant will ever change, let's just use it instead of making calls to get the same information.
@smmalis37 smmalis37 deleted the patch-1 branch February 18, 2024 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChrisDenton ChrisDenton ChrisDenton left review comments

@deltragon deltragon deltragon requested changes

Assignees

@ChrisDenton ChrisDenton

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
Milestone
1.78.0
Development

Successfully merging this pull request may close these issues.
8 participants
@smmalis37 @rustbot @rust-log-analyzer @deltragon @ChrisDenton @Urgau @bors @thomcc