[PATCH] D83772: [Windows] Fix limit on command line size

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

I'm jumping in since rnk is on leave and (I believe) zturner is less focused on these issues than he used to be.

Thanks for tracking down the cause of this bug.

I have some concerns:

- We're trying to cut it right up to the hard limit.  That seems an unnecessary risk, as the off-by-one bug illustrates.  Is there anything wrong with just rounding down to 32,000 and leaving ourselves some wiggle room?  Will someone be upset that we used a response file for a _very_ long command that technically would have been a viable command line?

- We're constructing a temporary copy of the command line to test if it's short enough and then constructing the actual command line elsewhere, which leaves us open to divergence between when we think the command line will be and what it actually ends up being.  I'd rather measure the actual command line, especially if we're going to run right up to the limit.

- We're checking to make sure that the number of UTF-8 code units is below the limit, but the limit is actually in terms of WCHARs.  Fortunately, I think that discrepancy works in our favor, since the number of WCHARs will always be smaller than the number of UTF-8 code units.  But it underscores the points I'm making above:  the precise limit probably isn't an issue and we're measuring a proxy command line rather than the actual command line.

Please consider these suggestions:

- Round down to 32,000 to leave us wiggle room.

- Have `flattenWindowsCommandLine` return a `wstring` rather than a `string`.  This will reduce the chance of the proxy string we measure differing from the actual command string we're issuing, and it's already a Windows-specific function.  This will require a change in Execute (in the same source file), which is currently doing the conversion from UTF-8 to UTF-16.

- Add a short command to the test for `commandLineFitsWithinSystemLimits`.  Right now, the test only checks a humongous command line.  (The test is in llvm\unittests\Support\CommandLineTest.cpp.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83772/new/

https://reviews.llvm.org/D83772