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

Response file parsing for GCC and Clang #1781

Conversation

Alexei-Barnes
Copy link
Contributor

This fixes #1780

This moves the response file parsing code implemented by @temportalflux out of the MSVC compiler and shares it for all compilers. This enables GCC and Clang to use response files with quoted arguments.

I've added some lib tests to the response file module, so that it can be reasoned about, separately from the compilers using it.

@codecov-commenter
Copy link

codecov-commenter commented May 31, 2023

Codecov Report

Patch coverage: 66.19% and project coverage change: -0.08% ⚠️

Comparison is base (7074753) 29.46% compared to head (3b2f394) 29.38%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1781      +/-   ##
==========================================
- Coverage   29.46%   29.38%   -0.08%     
==========================================
  Files          49       50       +1     
  Lines       17844    17872      +28     
  Branches     8631     8625       -6     
==========================================
- Hits         5257     5252       -5     
- Misses       7347     7356       +9     
- Partials     5240     5264      +24     
Files Changed Coverage Δ
src/compiler/gcc.rs 53.88% <0.00%> (+0.14%) ⬆️
src/compiler/msvc.rs 44.98% <0.00%> (-0.13%) ⬇️
src/compiler/response_file.rs 70.14% <70.14%> (ø)

... and 12 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sylvestre
Copy link
Collaborator

rustfmt isn't happy, could you please fix it? thanks

@Alexei-Barnes Alexei-Barnes force-pushed the response-file-parsing-for-clang-and-gcc branch from 40624f9 to 27b601f Compare June 1, 2023 09:12
@Alexei-Barnes
Copy link
Contributor Author

Ran cargo fmt and force pushed.

@sylvestre sylvestre force-pushed the response-file-parsing-for-clang-and-gcc branch 2 times, most recently from 4b14306 to b985902 Compare June 1, 2023 14:58
@@ -0,0 +1,165 @@
/// An iterator over the arguments in a response file.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would prefer the move to be done into a single commit
and then, modifications into a second commit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll rewrite the commit into two commits in that order.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Alexei-Barnes ping ? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the slow action here - I've been pulled into a work project that's eating my free time. I should get back to this soon.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you still going to work on this? thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, sorry for the mega wait - I'm working on this again now.

Copy link
Contributor Author

@Alexei-Barnes Alexei-Barnes Aug 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this is split into two separate commits now. The first commit just moves the code, with minimal changes necessary to do so. The second change modifies it, adds tests, and uses it in the GCC module. The final code is unchanged from when you last looked at it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now also merged main into this branch, to keep it up to date. No merge conflicts.

* Moves the MSVC response file code into a new module
* Makes no† changes to the response file code

† Had to make `SplitMsvcResponseFileArgs` `pub`, so that it can be used.
* Remove anything specific about MSVC from the response file code (which is just the name and a comment)
* Add unit tests to the module
* Enable use of the response file code in gcc/clang (via the gcc module, clang defaults to this behaviour)
@Alexei-Barnes Alexei-Barnes force-pushed the response-file-parsing-for-clang-and-gcc branch from b985902 to 1562001 Compare August 16, 2023 08:38
@sylvestre
Copy link
Collaborator

sorry but it will need to be rebase :(

@robertmaynard
Copy link
Collaborator

What is the value of parsing an response file instead of just hashing it? Is this ensures we are invariant to order of flags in the response file?

@glandium
Copy link
Collaborator

glandium commented Nov 28, 2023

What is the value of parsing an response file instead of just hashing it? Is this ensures we are invariant to order of flags in the response file?

For the same reasons we don't just hash command line arguments:

  • some flags need to be handled in a special way
  • some flags need to disable caching because we can't handle them
  • we need to distinguish preprocessor flags from other flags so that we can invoke the preprocessor with the right flags
  • the input file may be in the response file, and we need to know what it is
  • etc.

@robertmaynard
Copy link
Collaborator

What is the value of parsing an response file instead of just hashing it? Is this ensures we are invariant to order of flags in the response file?

For the same reasons we don't just hash command line arguments:

  • some flags need to be handled in a special way
  • some flags need to disable caching because we can't handle them
  • we need to distinguish preprocessor flags from other flags so that we can invoke the preprocessor with the right flags
  • the input file may be in the response file, and we need to know what it is
  • etc.

Makes sense. My feedback would be we should extend ArgData in gcc.rs to have a explicit type for response files.
This would allow compilers that extend gcc such as nvcc to leverage this work even when they have a different command line syntax to represent response files.

@sylvestre
Copy link
Collaborator

no activity for a while, closing

@sylvestre sylvestre closed this Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Response file parsing for GCC and Clang does not handle quoted arguments
5 participants