[PATCH] D73060: [Clang] Fix expansion of response files in -Wp after integrated-cc1 change

[Alexandre Ganea via Phabricator via cfe-commits]

aganea abandoned this revision.
aganea added a comment.

Abandoning this in favor of D73120 . Please 
take a look at the other patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73060



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73060: [Clang] Fix expansion of response files in -Wp after integrated-cc1 change

[Hans Wennborg via Phabricator via cfe-commits]

hans added inline comments.



Comment at: clang/tools/driver/driver.cpp:338
+  static unsigned ReenteranceCount;
+  if (ReenteranceCount++)
+llvm::cl::ResetAllOptionOccurrences();

aganea wrote:
> hans wrote:
> > This looks pretty hacky.
> > 
> > Can you explain more what the current problem is and how you're proposing 
> > to fix it? I'm not familiar with the -Wp flag and how it relates to 
> > response files.
> The problem is that for `-Wp,@file.rsp`, file.rsp is not expanded anymore, 
> because previously it was done in the cc1 process, at L379 below.
> 
> This patch calls back into `ExecuteClangTool` instead of `ExecuteCC1Tool`. 
> This was my initial proposal for rGb4a99a061f517e60985667e39519f60186cbb469, 
> purposely to fix this issue (which I forgot about)
> 
> An alternate way is to duplicate L379 into `ExecuteCC1Tool`, but I felt that 
> wasn't pretty and opens the door to more code duplication down the line. See 
> D73120 for an alternate implementation.
I think that if cc1 is supposed to expand response files, then let's make that 
explicit.

I don't think the "reenter" flag in D73120 is necessary: if cc1 is supposed to 
expand response files, let's make it do that always.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73060



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73060: [Clang] Fix expansion of response files in -Wp after integrated-cc1 change

[Alexandre Ganea via Phabricator via cfe-commits]

aganea marked an inline comment as done.
aganea added inline comments.



Comment at: clang/tools/driver/driver.cpp:338
+  static unsigned ReenteranceCount;
+  if (ReenteranceCount++)
+llvm::cl::ResetAllOptionOccurrences();

hans wrote:
> This looks pretty hacky.
> 
> Can you explain more what the current problem is and how you're proposing to 
> fix it? I'm not familiar with the -Wp flag and how it relates to response 
> files.
The problem is that for `-Wp,@file.rsp`, file.rsp is not expanded anymore, 
because previously it was done in the cc1 process, at L379 below.

This patch calls back into `ExecuteClangTool` instead of `ExecuteCC1Tool`. This 
was my initial proposal for rGb4a99a061f517e60985667e39519f60186cbb469, 
purposely to fix this issue (which I forgot about)

An alternate way is to duplicate L379 into `ExecuteCC1Tool`, but I felt that 
wasn't pretty and opens the door to more code duplication down the line. See 
D73120 for an alternate implementation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73060



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73060: [Clang] Fix expansion of response files in -Wp after integrated-cc1 change

[Hans Wennborg via Phabricator via cfe-commits]

hans added inline comments.



Comment at: clang/tools/driver/driver.cpp:338
+  static unsigned ReenteranceCount;
+  if (ReenteranceCount++)
+llvm::cl::ResetAllOptionOccurrences();

This looks pretty hacky.

Can you explain more what the current problem is and how you're proposing to 
fix it? I'm not familiar with the -Wp flag and how it relates to response files.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73060



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73060: [Clang] Fix expansion of response files in -Wp after integrated-cc1 change

[Wenlei He via Phabricator via cfe-commits]

wenlei accepted this revision.
wenlei added a comment.
This revision is now accepted and ready to land.

> Is that the desired behavior? This was the previous behavior but I'm not sure 
> it's the right behavior.

Good point. It might be safer to keep that previous behavior though.

Thanks for the quick fix, LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73060



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73060: [Clang] Fix expansion of response files in -Wp after integrated-cc1 change

[Alexandre Ganea via Phabricator via cfe-commits]

aganea created this revision.
aganea added reviewers: hans, thakis, wenlei.
aganea added a project: clang.
Herald added a subscriber: cfe-commits.
aganea edited the summary of this revision.

After rGb4a99a061f517e60985667e39519f60186cbb469 
, passing 
a response file such as `-Wp,@a.rsp` wasn't working anymore because .rsp 
expansion happens inside clang's main() function.

I'm noticing along the way that explicit arguments in `-Wp` are parsed in the 
driver, whereas arguments inside a response file are parsed in the -cc1 tool. 
Is that the desired behavior? This was the previous behavior but I'm not sure 
it's the right behavior.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73060

Files:
  clang/include/clang/Driver/Driver.h
  clang/lib/Driver/Job.cpp
  clang/test/Driver/Wp-args.c
  clang/tools/driver/driver.cpp

Index: clang/tools/driver/driver.cpp
===
--- clang/tools/driver/driver.cpp
+++ clang/tools/driver/driver.cpp
@@ -241,8 +241,6 @@
   *NumberSignPtr = '=';
 }
 
-static int ExecuteCC1Tool(ArrayRef argv);
-
 static void SetBackdoorDriverOutputsFromEnvVars(Driver ) {
   // Handle CC_PRINT_OPTIONS and CC_PRINT_OPTIONS_FILE.
   TheDriver.CCPrintOptions = !!::getenv("CC_PRINT_OPTIONS");
@@ -318,7 +316,6 @@
   // Driver::CC1Main), we need to clean up the options usage count. The options
   // are currently global, and they might have been used previously by the
   // driver.
-  llvm::cl::ResetAllOptionOccurrences();
   StringRef Tool = argv[1];
   void *GetExecutablePathVP = (void *)(intptr_t) GetExecutablePath;
   if (Tool == "-cc1")
@@ -334,17 +331,13 @@
   return 1;
 }
 
-int main(int argc_, const char **argv_) {
-  noteBottomOfStack();
-  llvm::InitLLVM X(argc_, argv_);
-  SmallVector argv(argv_, argv_ + argc_);
-
-  if (llvm::sys::Process::FixupStandardFileDescriptors())
-return 1;
-
-  llvm::InitializeAllTargets();
+int ExecuteClangTool(SmallVectorImpl ) {
   auto TargetAndMode = ToolChain::getTargetAndModeFromProgramName(argv[0]);
 
+  static unsigned ReenteranceCount;
+  if (ReenteranceCount++)
+llvm::cl::ResetAllOptionOccurrences();
+
   llvm::BumpPtrAllocator A;
   llvm::StringSaver Saver(A);
 
@@ -480,7 +473,7 @@
   SetBackdoorDriverOutputsFromEnvVars(TheDriver);
 
   if (!UseNewCC1Process) {
-TheDriver.CC1Main = 
+TheDriver.CC1Main = 
 // Ensure the CC1Command actually catches cc1 crashes
 llvm::CrashRecoveryContext::Enable();
   }
@@ -543,3 +536,15 @@
   // failing command.
   return Res;
 }
+
+int main(int argc_, const char **argv_) {
+  noteBottomOfStack();
+  llvm::InitLLVM X(argc_, argv_);
+  SmallVector argv(argv_, argv_ + argc_);
+
+  if (llvm::sys::Process::FixupStandardFileDescriptors())
+return 1;
+
+  llvm::InitializeAllTargets();
+  return ExecuteClangTool(argv);
+}
Index: clang/test/Driver/Wp-args.c
===
--- clang/test/Driver/Wp-args.c
+++ clang/test/Driver/Wp-args.c
@@ -19,3 +19,9 @@
 // MMD: "-cc1"
 // MMD-NOT: -MMD
 // MMD: "-dependency-file" "Wp-args.d"
+
+// Ensure response files are properly expanded with -Wp
+// RUN: echo -rewrite-macros > %t.rsp
+// RUN: %clang -Wp,@%t.rsp -E %s | FileCheck -check-prefix RSP %s
+
+// RSP: inception
Index: clang/lib/Driver/Job.cpp
===
--- clang/lib/Driver/Job.cpp
+++ clang/lib/Driver/Job.cpp
@@ -384,7 +384,6 @@
   SmallVector Argv;
   Argv.push_back(getExecutable());
   Argv.append(getArguments().begin(), getArguments().end());
-  Argv.push_back(nullptr);
 
   // This flag simply indicates that the program couldn't start, which isn't
   // applicable here.
Index: clang/include/clang/Driver/Driver.h
===
--- clang/include/clang/Driver/Driver.h
+++ clang/include/clang/Driver/Driver.h
@@ -208,7 +208,7 @@
   /// When the clangDriver lib is used through clang.exe, this provides a
   /// shortcut for executing the -cc1 command-line directly, in the same
   /// process.
-  typedef int (*CC1ToolFunc)(ArrayRef argv);
+  typedef int (*CC1ToolFunc)(SmallVectorImpl &);
   CC1ToolFunc CC1Main = nullptr;
 
 private:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits