Long argsfile doesn't work properly on Windows #79923

KapJI · 2020-12-11T02:18:08Z

Example: https://github.com/KapJI/rust_example
Commands to repro: https://github.com/KapJI/rust_example/blob/main/test.sh
Example CI job: https://github.com/KapJI/rust_example/runs/1534666156

With shorter argsfile it compiled correctly but longer one failed with

error: cannot determine resolution for the import
 --> b/lib.rs:1:5
  |
1 | use a::A;
  |     ^^^^
  |
  = note: import resolution is stuck, try simplifying other imports

which is very obscure and not really relevant.

Note that I added many duplicated lines in argsfiles to demonstrate that it starts to fail after some threshold, actual content is not important. In the real project we have different lines for different dependencies.

If you comment

pub trait SerdeWith<SerdeType> {
    fn deserialize<'a, D: ::serde::Deserializer<'a>>(
        deserializer: D,
    ) -> Result<Self, D::Error>
    where
        Self: Sized;
}

in a/lib.rs it will work in both cases. But this trait is actually not used in this example.

At some point I also got internal compiler error, not sure if it's relevant:

thread 'rustc' panicked at 'index out of bounds: the len is 236 but the index is 236', compiler\rustc_codegen_ssa\src\back\symbol_export.rs:292:13

The text was updated successfully, but these errors were encountered:

KapJI · 2020-12-12T16:28:51Z

I also checked that it doesn't fail neither on Mac nor Linux even if argsfile is 100 times bigger.

Also it seems that it starts to fail not because of large argsfile itself but because of total length of paths in -Ldependency args. If you replace relative paths with absolute it starts failing earlier even if file size is smaller.
It's also not number of entries, for shorter paths like in my example you can add more lines before it starts failing and file size will be bigger. It's probably not really related to argsfile but without it you can't have long enough list of arguments on Windows.

jsgf · 2020-12-14T19:45:03Z

That array bounds error is from:

rust/compiler/rustc_codegen_ssa/src/back/symbol_export.rs

Lines 287 to 296 in fa41639

    
           let cnum_stable_ids: IndexVec<CrateNum, Fingerprint> = { 
        
               let mut cnum_stable_ids = IndexVec::from_elem_n(Fingerprint::ZERO, cnums.len() + 1); 
        
               for &cnum in cnums.iter() { 
        
                   cnum_stable_ids[cnum] = 
        
                       tcx.def_path_hash(DefId { krate: cnum, index: CRATE_DEF_INDEX }).0; 
        
               } 
        
               cnum_stable_ids 
        
           };

which seems to be assuming that all cnum in cnums are < cnums.len(). No idea why that assumption is 1) valid and 2) seems to fail in this case.

Should it actually be:

     let mut cnum_stable_ids = IndexVec::from_elem_n(Fingerprint::ZERO, cnums.iter().max() + 1);

?

Summary: Attempt to handle rust-lang/rust#79923 on our side which probably won't be fixed very soon. The issue there with length of argsfile with dependency arguments (or actually with the total length of paths to them). Relative paths are obviously much shorter and it's enough to not hit this issue right now. Note that `dependentFilesystem` shouldn't be added to rule key in `RustLibraryArg`, otherwise rule key computation will fail. Reviewed By: jsgf fbshipit-source-id: c8bd2dbfb2982642010c4af1c7da3a637c87eab5

AustinWise · 2021-03-09T04:17:26Z

For what it's worth, I added a batch file version of the reproduction script. This way you can reproduce the problem without having to use a unix shell:

https://github.com/KapJI/rust_example/blob/main/test.cmd

KapJI · 2021-10-27T12:19:18Z

I did some debugging and here is what's going on there.

All directories from library search path are added to PATH on Windows:

rust/compiler/rustc_interface/src/passes.rs

Lines 280 to 309 in 47aeac6

    
           // Windows dlls do not have rpaths, so they don't know how to find their 
        
           // dependencies. It's up to us to tell the system where to find all the 
        
           // dependent dlls. Note that this uses cfg!(windows) as opposed to 
        
           // targ_cfg because syntax extensions are always loaded for the host 
        
           // compiler, not for the target. 
        
           // 
        
           // This is somewhat of an inherently racy operation, however, as 
        
           // multiple threads calling this function could possibly continue 
        
           // extending PATH far beyond what it should. To solve this for now we 
        
           // just don't add any new elements to PATH which are already there 
        
           // within PATH. This is basically a targeted fix at #17360 for rustdoc 
        
           // which runs rustc in parallel but has been seen (#33844) to cause 
        
           // problems with PATH becoming too long. 
        
           let mut old_path = OsString::new(); 
        
           if cfg!(windows) { 
        
               old_path = env::var_os("PATH").unwrap_or(old_path); 
        
               let mut new_path = sess.host_filesearch(PathKind::All).search_path_dirs(); 
        
               for path in env::split_paths(&old_path) { 
        
                   if !new_path.contains(&path) { 
        
                       new_path.push(path); 
        
                   } 
        
               } 
        
               env::set_var( 
        
                   "PATH", 
        
                   &env::join_paths( 
        
                       new_path.iter().filter(|p| env::join_paths(iter::once(p)).is_ok()), 
        
                   ) 
        
                   .unwrap(), 
        
               ); 
        
           }

And it gets very long. Documentation says that the maximum length for environment variable is 32,767 and there is no limit on the total size of environment block in recent versions of Windows:
https://docs.microsoft.com/en-us/windows/win32/procthread/environment-variables

It doesn't seem to be the whole truth though. In my case the issue starts when PATH becomes 32,682 chars long while 32,664 still works.

SetEnvironmentVariableW succeeds in both cases and GetEnvironmentVariableW correctly returns the new value. It actually succeeds even if the new value is longer than 32,767 which is strange.

But after setting the new PATH calls to LoadLibraryW start to fail with Not enough memory resources are available to process this command. (os error 8). CreateProcess also fails, probably some other Win32 calls which use PATH start to fail as well.

In this example dependencies look like: b => a => serde => serde_derive. serde_derive is proc macro and to register it creader calls LoadLibraryW to load DLL:

rust/compiler/rustc_metadata/src/creader.rs

Line 674 in 47aeac6

let lib = match DynamicLibrary::open(&path) {

That call fails and import resolution ends up in some bad state.

Some problems I noticed

When PATH is updated, new size is not checked. It'd great to fail early with some clear error message there. Although it's not easy to check this because even in this example the new PATH is close but still below the limit.
When LoadLibraryW fails, the error is hidden from user. It also could help to debug it quicker.
Search paths from -L flags are not deduped. In the real case for us they're all different but still a small optimisation to consider.

@alexcrichton as you implemented this logic in the first place, is there something that can be improved on rustc side?

alexcrichton · 2021-10-27T14:14:46Z

Sorry this is all so far out of cache for me I fear I won't be of much use. All I remember is that blocks like that in the compiler are really only there to make things work, so if things work without a block like that or if blocks like that are altered and things still work that's probably fine then. It's unlikely rustc needs to do exactly what it does today to keep working.

ChrisDenton · 2021-10-27T23:30:16Z

RE: The comment that says "Windows dlls do not have rpaths". That's true but also not quite right. Configuration manifests allow setting probing paths. However, they're limited to either subdirectories or a maximum or two levels above the directory. Though I'm not sure if this information is useful for this use case (and it's possible they might be length limited too, I don't know off hand).

On the environment limits. I'm not entirely surprised that ~32k is still an issue, unfortunately. The kernel uses structures like UNICODE_STRING which are limited to u16 lengths (which is in bytes, and a UTF-16 code unit is two bytes). So any function that just-so-happens to (at some point) pass the string in a structure like that should error if the length would overflow.

Summary: See rust-lang/rust#79923 for the motivation, we replicate what happens with cargo Reviewed By: jdonald, jsgf fbshipit-source-id: 9c9ac1c8c99f13014ecf4d552ad2a7690c0e2873

KapJI added the C-bug Category: This is a bug. label Dec 11, 2020

camelid added the O-windows Operating system: Windows label Dec 11, 2020

KapJI mentioned this issue Mar 8, 2021

Add a batch file reproduction script KapJI/rust_example#1

Merged

timbess linked a pull request Aug 15, 2023 that will close this issue

Fix argfile length rustc bug (Windows) bazelbuild/rules_rust#2116

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Long argsfile doesn't work properly on Windows #79923

Long argsfile doesn't work properly on Windows #79923

KapJI commented Dec 11, 2020

KapJI commented Dec 12, 2020 •

edited

Loading

jsgf commented Dec 14, 2020

AustinWise commented Mar 9, 2021 •

edited

Loading

KapJI commented Oct 27, 2021 •

edited

Loading

alexcrichton commented Oct 27, 2021

ChrisDenton commented Oct 27, 2021 •

edited

Loading

Long argsfile doesn't work properly on Windows #79923

Long argsfile doesn't work properly on Windows #79923

Comments

KapJI commented Dec 11, 2020

KapJI commented Dec 12, 2020 • edited Loading

jsgf commented Dec 14, 2020

AustinWise commented Mar 9, 2021 • edited Loading

KapJI commented Oct 27, 2021 • edited Loading

Some problems I noticed

alexcrichton commented Oct 27, 2021

ChrisDenton commented Oct 27, 2021 • edited Loading

KapJI commented Dec 12, 2020 •

edited

Loading

AustinWise commented Mar 9, 2021 •

edited

Loading

KapJI commented Oct 27, 2021 •

edited

Loading

ChrisDenton commented Oct 27, 2021 •

edited

Loading