Add support for windows linker args#1670
Add support for windows linker args#1670n1ght-hunter wants to merge 1 commit intowild-linker:mainfrom
Conversation
| arch: default_target_arch(), | ||
| // Infrastructure | ||
| should_fork: true, | ||
| arch: const { Architecture::from_target_lexicon(target_lexicon::HOST.architecture) }, |
There was a problem hiding this comment.
the default should probably be the build target architecture not the build host
f5586b2 to
49fd37a
Compare
49fd37a to
e841eb9
Compare
There was a problem hiding this comment.
I'd prefer not to have a separate file for consts. We don't do that kind of thing elsewhere. It's a bit like having a module called "traits". It looks like these constants are a mix of ELF-specific constants and constants that should just be in the top-level args module.
There was a problem hiding this comment.
Github is showing the entire file as new, which means we can't see what has changed. I'd say either leave all the linux argument parsing in args.rs, or we could do a PR before this one where we just copy the file and don't edit it.
| /// - `-m <emulation>` — overrides format to ELF when present | ||
| /// | ||
| /// Priority: `-m` overrides format from `--target`. Architecture comes from `--target` only. | ||
| pub(crate) fn detect_target(args: &[String]) -> Result<DetectedTarget> { |
There was a problem hiding this comment.
I think the --target flag might be too much for this PR, especially since it's non-standard. I worry that the discussion might get bogged down. What about splitting that to a later PR and for this one just accepting --flavor gnu or --flavor link as the first argument like LLD does?
| .long("APPCONTAINER") | ||
| .help("/APPCONTAINER - Specifies whether the app must run within an appcontainer process environment.") | ||
| .execute(|_, _| unimplemented_option("/APPCONTAINER")); | ||
| // /ARM64XFUNCTIONPADMINX64 - Specifies the minimum number of bytes of padding between x64 functions in ARM64X images. 17.8 |
There was a problem hiding this comment.
The comments seem to duplicate the help text. Also, they seem to be copied from Microsoft. I suspect for copyright reasons, we need to come up with our own descriptions of each flag. For unimplemented flags, it'd be fine to have the help text just say "unimplemented" and we can fill in our own descriptions as we implement them.
| r#"C:\Users\Samuel\AppData\Local\Temp\rustc7RL5Io\symbols.o"#, | ||
| "dummy.dummy.6cfbe55db138f4b-cgu.0.rcgu.o", | ||
| "dummy.3wxfnlvokcqcl6j45c8xeicgz.rcgu.o", | ||
| r#"C:\Users\Samuel\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\x86_64-pc-windows-msvc\lib\libstd-efa6c7783284bd31.rlib"#, |
There was a problem hiding this comment.
Perhaps edit these lines to make them a bit shorter.
|
im pretty busy with work stuff but will get to this over the weekend |
mati865
left a comment
There was a problem hiding this comment.
Few things I noticed during a quick skim through this PR.
| pub(crate) nx_compat: bool, | ||
| pub(crate) terminal_server_aware: bool, | ||
| pub(crate) high_entropy_va: bool, | ||
| pub(crate) no_default_libs: Vec<String>, |
There was a problem hiding this comment.
This looks like GNU's -nodefaultlibs but works very differently. I'd suggest renaming it to disabled_default_libs or something like that.
| /// and PE-specific fields are accessible via `Deref`/`DerefMut`. | ||
| #[derive(Debug)] | ||
| pub struct PeArgs { | ||
| // Windows-specific fields |
There was a problem hiding this comment.
This comment doesn't seem useful - looks like it applies to all struct fields.
| if std::env::var(REFERENCE_LINKER_ENV).is_ok() { | ||
| args.write_layout = true; | ||
| args.write_trace = true; | ||
| } |
| let files_per_group: Option<u32> = std::env::var(FILES_PER_GROUP_ENV) | ||
| .ok() | ||
| .map(|s| s.parse()) | ||
| .transpose()?; | ||
|
|
||
| if let Some(x) = files_per_group { | ||
| ensure!( | ||
| x <= MAX_FILES_PER_GROUP, | ||
| "{FILES_PER_GROUP_ENV}={x} but maximum is {MAX_FILES_PER_GROUP}" | ||
| ); | ||
| } | ||
|
|
| } | ||
|
|
||
| pub(crate) fn setup_windows_argument_parser() -> ArgumentParser<PeArgs> { | ||
| // Helper function for unimplemented options |
There was a problem hiding this comment.
LLMs create unnecessary verbose comments for obvious things like this one. They only needlessly make files longer.
| use crate::args::InputSpec; | ||
| use std::path::Path; | ||
|
|
||
| // Example Windows linker flags from Rust compilation |
There was a problem hiding this comment.
This is misleading, Rust doesn't pass --target to the linkers. Using synthetic command line arguments rather than real-world will be easier to deal with.
| assert!(args.debug_info); // /DEBUG flag | ||
| assert!(args.nx_compat); // /NXCOMPAT flag |
There was a problem hiding this comment.
Comments like that are not useful.
| linker-layout = { path = "../linker-layout", version = "0.8.0" } | ||
| linker-trace = { path = "../linker-trace", version = "0.8.0" } | ||
| linker-utils = { path = "../linker-utils", version = "0.8.0" } | ||
| target-lexicon = { workspace = true } |
There was a problem hiding this comment.
Move it near to other t.. deps.
| leb128 = "0.2.5" | ||
| libc = "0.2.171" | ||
| libloading = "0.9.0" | ||
| target-lexicon = "0.13.5" |
| } | ||
| } | ||
|
|
||
| fn declare(&mut self) -> OptionDeclaration<'_, NoParam> { | ||
| #[must_use] | ||
| pub fn new_case_insensitive() -> Self { |
There was a problem hiding this comment.
This is link.exe variant, not just case-insensitive.
thanks so much. will should be able to clean it all up tm. i was hoping to have more time but work had some major issues that took up my time |
Requires #1629 to be merged before this can.
This pr adds windows style flags to the args parsing.
It also moves linux flags to linux modules.
This introduces target-lexicon as a new dep. for parsing -target which follows the llvm target triple spec.
Questions to be answered.