[PATCH] D100755: [llvm-rc] [3/4] Run clang to preprocess input files

2021-04-21 Thread Nico Weber via Phabricator via cfe-commits 
thakis added a comment.

Purely FYI: the `--` in the test isn't needed just on macOS but also on Linux 
if your checkout is eg under /usr: http://45.33.8.238/linux/44652/step_7.txt

(clang-cl has some nice diag if the /U flag is an existing file, since that 
happens fairly often. Maybe llvm-rc could have a nicer diag in that case too 
and suggest `--`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100755

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

[PATCH] D100755: [llvm-rc] [3/4] Run clang to preprocess input files

2021-04-21 Thread Martin Storsjö via Phabricator via cfe-commits 
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG64bc44f5ddfb: [llvm-rc] Run clang to preprocess input files 
(authored by mstorsjo).

Changed prior to commit:
  https://reviews.llvm.org/D100755?vs=338969=339141#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100755

Files:
  clang/test/CMakeLists.txt
  clang/test/Preprocessor/Inputs/llvm-rc.h
  clang/test/Preprocessor/llvm-rc.rc
  clang/test/lit.cfg.py
  llvm/test/tools/llvm-rc/absolute.test
  llvm/test/tools/llvm-rc/codepage.test
  llvm/test/tools/llvm-rc/cpp-output.test
  llvm/test/tools/llvm-rc/flags.test
  llvm/test/tools/llvm-rc/helpmsg.test
  llvm/test/tools/llvm-rc/include-paths.test
  llvm/test/tools/llvm-rc/language.test
  llvm/test/tools/llvm-rc/memoryflags-stringtable.test
  llvm/test/tools/llvm-rc/memoryflags.test
  llvm/test/tools/llvm-rc/not-expr.test
  llvm/test/tools/llvm-rc/parser-expr.test
  llvm/test/tools/llvm-rc/parser.test
  llvm/test/tools/llvm-rc/preproc.test
  llvm/test/tools/llvm-rc/tag-accelerators.test
  llvm/test/tools/llvm-rc/tag-dialog.test
  llvm/test/tools/llvm-rc/tag-escape.test
  llvm/test/tools/llvm-rc/tag-html.test
  llvm/test/tools/llvm-rc/tag-icon-cursor.test
  llvm/test/tools/llvm-rc/tag-menu.test
  llvm/test/tools/llvm-rc/tag-stringtable.test
  llvm/test/tools/llvm-rc/tag-user.test
  llvm/test/tools/llvm-rc/tag-versioninfo.test
  llvm/test/tools/llvm-rc/tokenizer.test
  llvm/test/tools/llvm-rc/versioninfo-padding.test
  llvm/tools/llvm-rc/Opts.td
  llvm/tools/llvm-rc/llvm-rc.cpp

Index: llvm/tools/llvm-rc/llvm-rc.cpp
===
--- llvm/tools/llvm-rc/llvm-rc.cpp
+++ llvm/tools/llvm-rc/llvm-rc.cpp
@@ -17,17 +17,22 @@
 #include "ResourceScriptStmt.h"
 #include "ResourceScriptToken.h"
 
+#include "llvm/ADT/Triple.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/FileUtilities.h"
+#include "llvm/Support/Host.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Process.h"
+#include "llvm/Support/Program.h"
 #include "llvm/Support/Signals.h"
+#include "llvm/Support/StringSaver.h"
 #include "llvm/Support/raw_ostream.h"
 
 #include 
@@ -71,12 +76,114 @@
 };
 
 static ExitOnError ExitOnErr;
+static FileRemover TempPreprocFile;
 
 LLVM_ATTRIBUTE_NORETURN static void fatalError(const Twine ) {
   errs() << Message << "\n";
   exit(1);
 }
 
+std::string createTempFile(const Twine , StringRef Suffix) {
+  std::error_code EC;
+  SmallString<128> FileName;
+  if ((EC = sys::fs::createTemporaryFile(Prefix, Suffix, FileName)))
+fatalError("Unable to create temp file: " + EC.message());
+  return static_cast(FileName);
+}
+
+ErrorOr findClang(const char *Argv0) {
+  StringRef Parent = llvm::sys::path::parent_path(Argv0);
+  ErrorOr Path = std::error_code();
+  if (!Parent.empty()) {
+// First look for the tool with all potential names in the specific
+// directory of Argv0, if known
+for (const auto *Name : {"clang", "clang-cl"}) {
+  Path = sys::findProgramByName(Name, Parent);
+  if (Path)
+return Path;
+}
+  }
+  // If no parent directory known, or not found there, look everywhere in PATH
+  for (const auto *Name : {"clang", "clang-cl"}) {
+Path = sys::findProgramByName(Name);
+if (Path)
+  return Path;
+  }
+  return Path;
+}
+
+std::string getClangClTriple() {
+  Triple T(sys::getDefaultTargetTriple());
+  T.setOS(llvm::Triple::Win32);
+  T.setVendor(llvm::Triple::PC);
+  T.setEnvironment(llvm::Triple::MSVC);
+  T.setObjectFormat(llvm::Triple::COFF);
+  return T.str();
+}
+
+bool preprocess(StringRef Src, StringRef Dst, opt::InputArgList ,
+const char *Argv0) {
+  std::string Clang;
+  if (InputArgs.hasArg(OPT__HASH_HASH_HASH)) {
+Clang = "clang";
+  } else {
+ErrorOr ClangOrErr = findClang(Argv0);
+if (ClangOrErr) {
+  Clang = *ClangOrErr;
+} else {
+  errs() << "llvm-rc: Unable to find clang, skipping preprocessing."
+ << "\n";
+  errs() << "Pass -no-cpp to disable preprocessing. This will be an error "
+"in the future."
+ << "\n";
+  return false;
+}
+  }
+  std::string PreprocTriple = getClangClTriple();
+
+  SmallVector Args = {
+  Clang, "--driver-mode=gcc", "-target", PreprocTriple, "-E",
+  "-xc", "-DRC_INVOKED",  Src,   "-o",  Dst};
+  if (InputArgs.hasArg(OPT_noinclude)) {
+#ifdef _WIN32
+::_putenv("INCLUDE=");
+#else
+::unsetenv("INCLUDE");
+#endif
+  }
+  for (const auto *Arg :
+   InputArgs.filtered(OPT_includepath,

[PATCH] D100755: [llvm-rc] [3/4] Run clang to preprocess input files

2021-04-20 Thread Alexandre Ganea via Phabricator via cfe-commits 
aganea accepted this revision.
aganea added a comment.

LGTM.




Comment at: llvm/tools/llvm-rc/llvm-rc.cpp:94
+
+ErrorOr findClang(const char *Argv0) {
+  StringRef Parent = llvm::sys::path::parent_path(Argv0);

mstorsjo wrote:
> aganea wrote:
> > Since you're not using the `std::error_code` below in the call site, should 
> > this return `Expected<...>`? In that case, the variable `Path` shouldn't be 
> > needed?
> With `Expected<>` I'd need to craft an `Error` at the end if I don't have a 
> path to return, but do you mean `Optional<>`? I guess that'd work - but as 
> `findProgramByName()` returns `ErrorOr` I kept the same 
> signature.
> 
> Even if returning `Optional<>`, we need a local variable to receive the 
> `ErrorOr` from `findProgramByName()` and inspect it.
> 
> In the future (after a release or so) I'd intend for this to be a hard error, 
> so at that point, the returned error code actually would be printed.
I see, thanks for the explanation!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100755

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

[PATCH] D100755: [llvm-rc] [3/4] Run clang to preprocess input files

2021-04-20 Thread Martin Storsjö via Phabricator via cfe-commits 
mstorsjo added inline comments.



Comment at: llvm/tools/llvm-rc/llvm-rc.cpp:94
+
+ErrorOr findClang(const char *Argv0) {
+  StringRef Parent = llvm::sys::path::parent_path(Argv0);

aganea wrote:
> Since you're not using the `std::error_code` below in the call site, should 
> this return `Expected<...>`? In that case, the variable `Path` shouldn't be 
> needed?
With `Expected<>` I'd need to craft an `Error` at the end if I don't have a 
path to return, but do you mean `Optional<>`? I guess that'd work - but as 
`findProgramByName()` returns `ErrorOr` I kept the same signature.

Even if returning `Optional<>`, we need a local variable to receive the 
`ErrorOr` from `findProgramByName()` and inspect it.

In the future (after a release or so) I'd intend for this to be a hard error, 
so at that point, the returned error code actually would be printed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100755

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

[PATCH] D100755: [llvm-rc] [3/4] Run clang to preprocess input files

2021-04-20 Thread Alexandre Ganea via Phabricator via cfe-commits 
aganea added inline comments.



Comment at: llvm/tools/llvm-rc/llvm-rc.cpp:94
+
+ErrorOr findClang(const char *Argv0) {
+  StringRef Parent = llvm::sys::path::parent_path(Argv0);

Since you're not using the `std::error_code` below in the call site, should 
this return `Expected<...>`? In that case, the variable `Path` shouldn't be 
needed?



Comment at: llvm/tools/llvm-rc/llvm-rc.cpp:154
+  Args.push_back("-U");
+  break;
+}

thakis wrote:
> mstorsjo wrote:
> > thakis wrote:
> > > Here's our chromium wrapper: 
> > > https://source.chromium.org/chromium/chromium/src/+/master:build/toolchain/win/rc/rc.py
> > > 
> > > On Windows, /winsysroot support and possibly -imsvc support would be nice 
> > > too.
> > Those sound useful - but I think I'd prefer to defer adding support for 
> > more nonstandard preprocessing options to a later patch.
> > 
> > What do you think of a generic `--preprocessor-arg` like in the windres 
> > frontend, which might be useful for @aganea?
> We don't need a general `--preprocessor-arg` as long as common args work.
> 
> Oh, on that note: 
> https://source.chromium.org/chromium/chromium/src/+/master:build/toolchain/win/rc/rc.py
>  also has `/showIncludes` support which is necessary to make ninja re-build 
> .rc files if either
> 
> * an included .h file is changed (needs preprocessor output)
> * an included resource file (.ico or similar) is changed (needs llvm-rc 
> support)
> 
> Don't know if llvm-rc has support for the latter part. If it doesn't, that'd 
> be nice to add, and some test that checks that both parts work would be nice. 
> (Neither in this patch; just thinking about what we'd need to switch.)
> We don't need a general --preprocessor-arg as long as common args work.
+1 with Nico. Native arguments in `llvm-rc` would be better (later).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100755

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

[PATCH] D100755: [llvm-rc] [3/4] Run clang to preprocess input files

2021-04-20 Thread Martin Storsjö via Phabricator via cfe-commits 
mstorsjo updated this revision to Diff 338969.
mstorsjo added a comment.
Herald added subscribers: cfe-commits, mgorny.
Herald added a project: clang.

- Added a testcase under clang/test/Preprocessor
- Changed the option to -no-preprocess
- Removed the env var for disabling preprocessing
- Made it look for both clang and clang-cl

This still uses --driver-mode=gcc; it currently doesn't allow setting options 
where --driver-mode=cl would matter, and allowing that does complicate things a 
little. So I'd leave that as a future improvement for when/if it becomes 
possible to set such options where it matters.

I'm also not adding any more convenience options like -winsysroot, -imsvc, 
-showincludes for now; leaving those to future patches (also possibly for 
people who actually use them who can verify that their use makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100755

Files:
  clang/test/CMakeLists.txt
  clang/test/Preprocessor/Inputs/llvm-rc.h
  clang/test/Preprocessor/llvm-rc.rc
  clang/test/lit.cfg.py
  llvm/test/tools/llvm-rc/absolute.test
  llvm/test/tools/llvm-rc/codepage.test
  llvm/test/tools/llvm-rc/cpp-output.test
  llvm/test/tools/llvm-rc/flags.test
  llvm/test/tools/llvm-rc/helpmsg.test
  llvm/test/tools/llvm-rc/include-paths.test
  llvm/test/tools/llvm-rc/language.test
  llvm/test/tools/llvm-rc/memoryflags-stringtable.test
  llvm/test/tools/llvm-rc/memoryflags.test
  llvm/test/tools/llvm-rc/not-expr.test
  llvm/test/tools/llvm-rc/parser-expr.test
  llvm/test/tools/llvm-rc/parser.test
  llvm/test/tools/llvm-rc/preproc.test
  llvm/test/tools/llvm-rc/tag-accelerators.test
  llvm/test/tools/llvm-rc/tag-dialog.test
  llvm/test/tools/llvm-rc/tag-escape.test
  llvm/test/tools/llvm-rc/tag-html.test
  llvm/test/tools/llvm-rc/tag-icon-cursor.test
  llvm/test/tools/llvm-rc/tag-menu.test
  llvm/test/tools/llvm-rc/tag-stringtable.test
  llvm/test/tools/llvm-rc/tag-user.test
  llvm/test/tools/llvm-rc/tag-versioninfo.test
  llvm/test/tools/llvm-rc/tokenizer.test
  llvm/test/tools/llvm-rc/versioninfo-padding.test
  llvm/tools/llvm-rc/Opts.td
  llvm/tools/llvm-rc/llvm-rc.cpp

Index: llvm/tools/llvm-rc/llvm-rc.cpp
===
--- llvm/tools/llvm-rc/llvm-rc.cpp
+++ llvm/tools/llvm-rc/llvm-rc.cpp
@@ -17,17 +17,22 @@
 #include "ResourceScriptStmt.h"
 #include "ResourceScriptToken.h"
 
+#include "llvm/ADT/Triple.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/FileUtilities.h"
+#include "llvm/Support/Host.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Process.h"
+#include "llvm/Support/Program.h"
 #include "llvm/Support/Signals.h"
+#include "llvm/Support/StringSaver.h"
 #include "llvm/Support/raw_ostream.h"
 
 #include 
@@ -71,12 +76,114 @@
 };
 
 static ExitOnError ExitOnErr;
+static FileRemover TempPreprocFile;
 
 LLVM_ATTRIBUTE_NORETURN static void fatalError(const Twine ) {
   errs() << Message << "\n";
   exit(1);
 }
 
+std::string createTempFile(const Twine , StringRef Suffix) {
+  std::error_code EC;
+  SmallString<128> FileName;
+  if ((EC = sys::fs::createTemporaryFile(Prefix, Suffix, FileName)))
+fatalError("Unable to create temp file: " + EC.message());
+  return static_cast(FileName);
+}
+
+ErrorOr findClang(const char *Argv0) {
+  StringRef Parent = llvm::sys::path::parent_path(Argv0);
+  ErrorOr Path = std::error_code();
+  if (!Parent.empty()) {
+// First look for the tool with all potential names in the specific
+// directory of Argv0, if known
+for (const auto Name : {"clang", "clang-cl"}) {
+  Path = sys::findProgramByName(Name, Parent);
+  if (Path)
+return Path;
+}
+  }
+  // If no Parent directory known, or not found there, look everywhere in PATH
+  for (const auto Name : {"clang", "clang-cl"}) {
+Path = sys::findProgramByName(Name);
+if (Path)
+  return Path;
+  }
+  return Path;
+}
+
+std::string getClangClTriple() {
+  Triple T(sys::getDefaultTargetTriple());
+  T.setOS(llvm::Triple::Win32);
+  T.setVendor(llvm::Triple::PC);
+  T.setEnvironment(llvm::Triple::MSVC);
+  T.setObjectFormat(llvm::Triple::COFF);
+  return T.str();
+}
+
+bool preprocess(StringRef Src, StringRef Dst, opt::InputArgList ,
+const char *Argv0) {
+  std::string Clang;
+  if (InputArgs.hasArg(OPT__HASH_HASH_HASH)) {
+Clang = "clang";
+  } else {
+ErrorOr ClangOrErr = findClang(Argv0);
+if (ClangOrErr) {
+  Clang = *ClangOrErr;
+} else {
+  errs() << "llvm-rc: Unable to find clang, skipping preprocessing."
+ << "\n";
+  errs() << "Pass -no-cpp to disable preprocessing. This will be an