rustc inside make -j2 warns (regression from 1.75.0)

[ojeda]

Non-recursive calls of rustc from Make in beta warn about the jobserver being there but not being usable. Thus users with calls to rustc will start to see warnings in their builds unless they start marking the calls as recursive.

Make fills MAKEFLAGS in all cases in 4.3, i.e. even for non-recursive cases (and in 4.4 only in some cases it passes an invalid pair). Is rustc going to try to always use the jobserver if it appears to be there even if -Zthreads is 1? The warning is nice to detect mistakenly non-recursive calls (and the Make manual asks for that), though, but it should probably be not emitted when rustc is behaving as sequential, i.e. users are not seeing the advantage, right?

Otherwise, if we want to consider this an early warning for projects to update their Makefiles, then I think the diagnostic text should provide a way to disable it easily without changes to the build system so that projects have time to adapt.

a:
	echo  'fn main() {}' | rustup run stable rustc -
	echo  'fn main() {}' | rustup run beta   rustc -
b:
	+echo 'fn main() {}' | rustup run stable rustc -
	+echo 'fn main() {}' | rustup run beta   rustc -

$ make -j2 a
echo  'fn main() {}' | rustup run stable rustc -
echo  'fn main() {}' | rustup run beta   rustc -
warning: failed to connect to jobserver from environment variable `MAKEFLAGS=" -j2 --jobserver-auth=3,4"`: cannot open file descriptor 3 from the jobserver environment variable value: Bad file descriptor (os error 9)
  |
  = note: the build environment is likely misconfigured

$ make -j2 b
echo 'fn main() {}' | rustup run stable rustc -
echo 'fn main() {}' | rustup run beta   rustc -

Warning added at #113730
Cc #113349
Cc @belovdv @nnethercote @oksbsb @bjorn3 @SparrowLii
@rustbot modify labels: +regression-from-stable-to-beta -regression-untriaged

[bjorn3]

Rustc uses parallelism for both frontend parallelism (currently disabled by default, configurable using -Zthreads=) and backend parallelism (doing optimizations on multiple codegen backends at the same time). Backend parallelism has been supported for years and is on by default. Rustc looks up the jobserver very early in the compilation process to ensure that if MAKEFLAGS references a non-existent fd rustc hasn't accidentally opened this fd before checking if it actually exists, which would cause rustc to read/write an arbitrary file, corrupting it. AFAIK there is no way to prevent it from trying to look up the jobserver other than unsetting MAKEFLAGS. You also would want rustc to respect the jobserver once rustc is no longer forced to use a single codegen unit through the use of --emit obj.

[ojeda]

Right, I should have added -Ccodegen-units=1 (we do that explicitly on top of --emit=obj and others).

The point is that similar users may wonder what rustc is trying to parallelize now (at upgrade time) when it was requested to behave sequentially.

I understand that it is simpler (and more flexible for the future) to tell users that they should always let rustc use the jobserver even when they pass -Ccodegen-units=1, future -Cthreads=1 and possibly other sources of parallelism in the future (which is why the warning is nice), but in that case, I would suggest mentioning this requirement and the jobserver in the user-facing rustc docs and adding a note in the diagnostic to those docs. It is already mentioned in Cargo's reference, but I don't see it in the rustc book.

To be clear, it is not an issue for us -- I will just pass the jobserver.

[the8472]

per-rustc parallelism seems like a red herring to me. In principle rustc could yield its implicit token to the jobserver even when it's running single-threaded.
The issue is that the environment instructs the process via MAKEFLAGS that there exists a jobserver which is passed via inherited file descriptors. This is verified at startup and found to be a lie, which can result in safety/correctness bugs.

A more robust way to do this is to use the named-fifo protocol where the environment variables can't go out of sync with descriptor inheritance. Support for that was added last year to jobserver-rs. But this requires gnu make 4.4

Passing the jobserver is not a requirement. Removing --jobserver-auth works too. It only warns when the environment variable and the file descriptors go out of sync.

[ojeda]

If I understand correctly, you are saying rustc could always take advantage of the jobserver. That is fine; however, what I am saying is that it should be documented that rustc may precisely do that, and thus recommend that users call rustc from Make as recursive.

Ideally, rustc should also describe what it is supposed to happen if things like -Cthreads/-Ccodegen-units are passed and the jobserver is also there, like Make suggests:

If you have a command-line argument controlling the parallel operation of your tool, consider whether your tool should detect situations where both the jobserver and the command-line argument are specified, and how it should react.

---

Passing the jobserver is not a requirement. Removing --jobserver-auth works too.

That is still a requirement: "mark it as recursive (recommended) or workaround make by removing the flag". In other words, what normal users should do is actually always call rustc as recursive if they want to support 4.3 and simple pipe mode in 4.4.

It only warns when the environment variable and the file descriptors go out of sync.

I guess it depends on what you include in "out of sync". Note that Make fills MAKEFLAGS with the jobserver regardless of whether it is marked as recursive or not (so the child will not be able to connnect). And in 4.4 in pipe mode it will even fill the variable twice in order to pass explicitly invalid descriptors if it considers the invocation to be non-recursive.

---

By the way, now that I take a closer look, I see other details that could use some tweaking:

• rustc warns when finding an unknown jobserver style, which the Make manual suggests to avoid (since new kinds may be introduced in the future):

  If your tool does not recognize the format of the --jobserver-auth string, it should assume the jobserver is using a different style and it cannot connect.

• rustc seems to use the first --jobserver-auth it finds, instead of the last, as Make asks. So, for instance, in the invalid descriptor case, I see it trying to open the first pair instead of the invalid pair:

  Be aware that the MAKEFLAGS variable may contain multiple instances of the --jobserver-auth= option. Only the last instance is relevant.

I should probably open another issue for that. These may be issues for jobserver, but others are on rustc's side, e.g. it should go with the default client if it gets CannotParse.

[the8472]

And in 4.4 in pipe mode it will even fill the variable twice in order to pass explicitly invalid descriptors if it considers the invocation to be non-recursive.

What values does it use? Are they can't-possibly-be-valid values, like -1? If so we should handle that as if they were not set.

[ojeda]

It uses (-2, -2). Yeah, you could explicitly handle those. Sadly, that only happens in 4.4 in pipe mode [*], which is why users likely want to call rustc as recursive if they intend to support 4.3:

a:
	@echo 'fn main() {}' | rustc -

$ make-4.4.1 -j2 --jobserver-style=pipe
warning: failed to connect to jobserver from environment variable `MAKEFLAGS=" -j2 --jobserver-auth=3,4 --jobserver-auth=-2,-2"`: cannot open file descriptor 3 from the jobserver environment variable value: Bad file descriptor (os error 9)
  |
  = note: the build environment is likely misconfigured

That test also shows that it attempts to use the first flag, not the last, too (it tries to open 3, not -2).

[*] Note that 4.4 only does it if it "considers" it non-recursive, e.g. if you have another command marked as recursive in the same recipe, the non-recursive will still be considered recursive... So you will again get the supposedly valid ones. It is quite confusing.

[ojeda]

I just filled the other two issues since they are separate.

I am happy to open the documentation one if we all agree it should be so (or we can reuse this one if you prefer).

[petrochenkov]

It uses (-2, -2).

IIRC, @belovdv planned to support the negative file descriptor case in jobserver-rs as make docs suggest (i.e. ignore, not warn), but the plans didn't materialize yet.

[weihanglo]

Are we planning to fix this and do beta backport?

[petrochenkov]

I think we can release a new version of jobserver-rs with the fixes, and then backport the version bump in rustc.

[ojeda]

Two are on rustc's side: this one (for which the solution may be documenting that rustc may always use the jobserver, and thus recommend users to always enable it & mark as recursive, if others agree) and #120532 (since it is about how the error is handled in rustc's side).

[apiraino]

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

[petrochenkov]

@weihanglo
I've released the new version of jobserver-rs, it can be updated in rustc and cargo now.

[ojeda]

Thanks Vadim!

[ojeda]

With #121564 merged (1.80), i.e. the documentation solution (https://doc.rust-lang.org/nightly/rustc/jobserver.html), I believe this one can be closed (the other issues we mentioned have their own issue).

One further improvement could perhaps be to add a note: see ... (or similar) in the diagnostic to point users to that new documentation.

[petrochenkov]

With #121564 merged (1.80), i.e. the documentation solution (https://doc.rust-lang.org/nightly/rustc/jobserver.html), I believe this one can be closed (the other issues we mentioned have their own issue).