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

REGRESSION: Meson Incorrectly Determines that a Response File is Unnecessary #13414

Closed
lhearachel opened this issue Jul 12, 2024 · 4 comments · Fixed by #13473
Closed

REGRESSION: Meson Incorrectly Determines that a Response File is Unnecessary #13414

lhearachel opened this issue Jul 12, 2024 · 4 comments · Fixed by #13473
Milestone

Comments

@lhearachel
Copy link

lhearachel commented Jul 12, 2024

Describe the bug

Meson seems to no longer correctly determine that a response file is necessary when using a compiler that runs under Wine. The identification routine worked as expected when building using Meson 1.4.1, but now fails in Meson 1.5.0 at the linking stage for our ELF.

The command that we use for our linking is 44_893 characters long; investigation into Wine's codebase suggests that their limit is 32_767 characters.

For further debugging under 1.5.0, I set MESON_RSP_THRESHOLD to 16387, and linking correctly identified that it should use a response file during project configuration.

To Reproduce
Our entire project source is available here. Instructions for building the project (if desired) are included in INSTALL.md.

Nothing extraordinary should be notable here other than:

  1. The number of source files incorporated into our LCF, and
  2. The fact that our compiler, linker, and assembler all have a dependency on Wine.

Expected behavior

The linker should identify that it should use a response file for a command that exceeds the supported length if running against Wine.

system parameters

  • Is this a cross build or just a plain native build (for the same computer)? Cross build.
  • what operating system (e.g. MacOS Catalina, Windows 10, CentOS 8.0, Ubuntu 18.04, etc.) Reproduced on Ubuntu 22.04 and Arch Linux
  • what Python version are you using e.g. 3.8.0 Reproduced on Python 3.8.2, 3.10.14, and 3.12.4
  • what meson --version 1.5.0
  • what ninja --version 1.12.1
@eli-schwartz
Copy link
Member

I can't think of an obviously suspicious change that might have caused this off the top of my head. Would you be able to try git bisecting meson to find out exactly when this stopped working?

@thesamesam
Copy link
Collaborator

cf0fecf is the first bad commit
commit cf0fecf
Author: Dylan Baker dylan@pnwbakers.com
Date: Thu Apr 18 14:20:07 2024 -0700

backend/ninja: Fix bug in NinjaRule.length_estimate

The code would create a dictionary that was of type `str : list[str] |
str | None`. Then would later try to call `len(' '.join(dict[key]))`.
This would result in two different bugs:

 1. If the value is `None` it would except, since None isn't iterable
    and cannot be converted to a string
 2. If the value was a string, then it would double the length of the
    actual string and return that, by adding a space between each
    character

mesonbuild/backend/ninjabackend.py | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)

cc @dcbaker

@dcbaker
Copy link
Member

dcbaker commented Jul 25, 2024

Okay, so after some debugging this appears to be "our bug was papering over another bug".

On Linux we set the line limit to 65k characters, but wine has a limit of 32k characters. My patch fixes a bug where we would double count most of the characters that went into our calculation, so we effectively had half the character limit we actually wanted (+/- ~1000 characters). In the linked project, the final command has a line length of ~44k characters, but on 1.4.0 that gets counted as 88k characters, and we use an RSP file.

I think the best solution is just to cap the line length at 32k for !Windows. We already cap Windows at 8k because that's the limit of cmd.exe, but it otherwise has a limit of 32k, just like Wine does.

dcbaker added a commit to dcbaker/meson that referenced this issue Jul 25, 2024
At an OS level, Unix-like OSes usually have very large or even
unlimited sized command line limits. In practice, however, many
applications do not handle this (intentionally or otherwise). Notably
Wine has the same limits Windows does, 32,768 characters. Because we
previously double counted most characters, we papered over most
situations that we would need an RSP file on Unix-like OSes with Wine.

To fix this issue I have set the command line limit to 32k, this is
still a massive command line to pass without an RSP file, and will only
cause the use of an RSP file where it is not strictly necessary in a
small number of cases, but will fix Wine applications. Projects who wish
to not use an RSP file can still set the MESON_RSP_THRESHOLD environment
variable to a very large number instead.

Fixes: mesonbuild#13414
Fixes: cf0fecf ("backend/ninja: Fix bug in NinjaRule.length_estimate")
@dcbaker dcbaker linked a pull request Jul 25, 2024 that will close this issue
@thesamesam
Copy link
Collaborator

Confirmed that #13473 fixes it.

eli-schwartz pushed a commit that referenced this issue Jul 25, 2024
At an OS level, Unix-like OSes usually have very large or even
unlimited sized command line limits. In practice, however, many
applications do not handle this (intentionally or otherwise). Notably
Wine has the same limits Windows does, 32,768 characters. Because we
previously double counted most characters, we papered over most
situations that we would need an RSP file on Unix-like OSes with Wine.

To fix this issue I have set the command line limit to 32k, this is
still a massive command line to pass without an RSP file, and will only
cause the use of an RSP file where it is not strictly necessary in a
small number of cases, but will fix Wine applications. Projects who wish
to not use an RSP file can still set the MESON_RSP_THRESHOLD environment
variable to a very large number instead.

Fixes: #13414
Fixes: cf0fecf ("backend/ninja: Fix bug in NinjaRule.length_estimate")
(cherry picked from commit a544c75)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants