[llvm-rc] Handle C preprocessor output

[mstorsjo]

Summary

When preprocessing resource scripts (which can easily be done outside of llvm-rc), included headers can leave behind C declarations (despite preprocessing with -DRC_INVOKED), that can't be parsed by a resource compiler.

This is handled in rc.exe (verified experimentally), GNU windres and wrc (clearly visible in the source) by parsing the preprocessor output line markers and ignoring content from files named *.h.

In addition to this filtering, strip out any other preprocessor directive that are left behind (like pragmas) which also can't be handled by the tokenizer.

The added test uses both standard #line markers (supported by rc.exe) and GNU style extended line markers, thus this test doesn't pass with rc.exe, but passes with GNU windres.

[rnk]

tools/llvm-rc/ResourceScriptCppFilter.cpp

54 ↗

(On Diff #145674)

This is O(n^2). This shouldn't need any copies until it's time to join the string, so you can make a std::vector<StringRef> and then use return llvm::join(Lines, ""), right?

[zturner]

tools/llvm-rc/ResourceScriptCppFilter.cpp

22 ↗

(On Diff #145674)

Mark with explicit.

34 ↗

(On Diff #145674)

Can you put these on different lines? Also, Pos and Outputting are uninitialized member variables, can we initialize them inline?

63 ↗

(On Diff #145674)

I think this would be simpler as if (!Line.startswith("#")) or perhaps even if (!Line.consume_front("#"))

71–86 ↗

(On Diff #145674)

I think all of this could be simplified to just a few lines.

Assuming you use the consume_front pattern above, then you could probably do the same here. Something like:

Line.consume_front("line");
if (!Line.starts_with(" "))
  return false;
Line = Line.ltrim();  // There could be multiple spaces after the #line directive

size_t N;
if (Line.consumeInteger(10, N))  // returns true to signify an error
  return false;

Line = Line.ltrim();

// Then consume the filename and last integer.

It's messy manually dealing with strings and substrings, so using as much of StringRef builtin functionality as possible would make this code much more readable.

90–91 ↗

(On Diff #145674)

Do we not actually *use* the line number for anything?

[amccarth]

tools/llvm-rc/ResourceScriptCppFilter.cpp

106 ↗

(On Diff #145674)

also "c"

tools/llvm-rc/ResourceScriptCppFilter.h

16 ↗

(On Diff #145674)

Also from files name *.c. See last paragraph on:

https://msdn.microsoft.com/en-us/library/windows/desktop/aa381033(v=vs.85).aspx

This is a hack from the old days. Instead of putting your UI IDs into a header, you'd just define them in the .c file that implements that part of the UI, knowing that the .c file could be included in the resource script.

[mstorsjo]

tools/llvm-rc/ResourceScriptCppFilter.cpp

22 ↗

(On Diff #145674)

Done

34 ↗

(On Diff #145674)

Sure, will initialize them inline here and split them over two lines.

54 ↗

(On Diff #145674)

That works just as well, thanks!

71–86 ↗

(On Diff #145674)

Thanks! Yes, I tried to use as much of the StringRef builtins as possible, but didn't really find the right ones that actually makes it this simple.

With more of ltrim, consume_front/consumeInteger and rsplit, I managed to make it really short and succinct.

90–91 ↗

(On Diff #145674)

Nope, I don't use it anywhere (yet).

If we'd want to have proper line numbers in error reports we'd need to keep these lines and handle them in the tokenizer somehow I guess, but I'm just aiming at the minimal functionality for this for now.

106 ↗

(On Diff #145674)

Done

tools/llvm-rc/ResourceScriptCppFilter.h

16 ↗

(On Diff #145674)

Whoa, color me surprised, I hadn't expected this to be documented.

[mstorsjo]

Adapted to the review comments; using std::vector<StringRef> + llvm::join to queue up the output, excluding *.c as well, added a reference to the documentation on the matter, simplified the line parsing significantly using @zturner's hints about better StringRef helpers to use.

llvm/trunk/test/tools/llvm-rc/Inputs/cpp-output.rc

+// Test that the input originally is included.
+STRINGTABLE {
+  1 "a"
+}
+#line 2 "cpp-source.rc"
+// Content from a rc file (potentially the source file itself) is included.
+STRINGTABLE {
+  2 "b"
+}
+// Test a preprocessing directive that starts with leading whitespace.
+  #line 1 "\\some\\path\\header.h"
+// Content from .h files is ignored.
+typedef int Foo;
+#line 123 "\\some\\path\\header.h"
+void someFunc(void);
+// Check GNU style line markers.
+# 4 "cpp-source.rc" 1
+STRINGTABLE {
+  3 "c"
+}
+  # 1 "other/header.h" 1
+typedef int Bar;
+# 10 "cpp-source.rc" 2
+// Test that other preprocessor directives are ignored.
+#pragma foo
+STRINGTABLE {
+  4 "d"
+}

llvm/trunk/test/tools/llvm-rc/cpp-output.test

+; RUN: llvm-rc /FO %t %p/Inputs/cpp-output.rc
+; RUN: llvm-readobj %t | FileCheck %s
+
+; CHECK:      Resource type (int): 6
+; CHECK-NEXT: Resource name (int): 1
+; CHECK-NEXT: Data version: 0
+; CHECK-NEXT: Memory flags: 0x1030
+; CHECK-NEXT: Language ID: 1033
+; CHECK-NEXT: Version (major): 0
+; CHECK-NEXT: Version (minor): 0
+; CHECK-NEXT: Characteristics: 0
+; CHECK-NEXT: Data size: 40
+; CHECK-NEXT: Data: (
+; CHECK-NEXT:   0000: 00000100 61000100 62000100 63000100  |....a...b...c...|
+; CHECK-NEXT:   0010: 64000000 00000000 00000000 00000000  |d...............|
+; CHECK-NEXT:   0020: 00000000 00000000                    |........|
+; CHECK-NEXT: )

llvm/trunk/tools/llvm-rc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Option
   Support
   )
 
 set(LLVM_TARGET_DEFINITIONS Opts.td)
 
 tablegen(LLVM Opts.inc -gen-opt-parser-defs)
 add_public_tablegen_target(RcTableGen)
 
 add_llvm_tool(llvm-rc
   llvm-rc.cpp
   ResourceFileWriter.cpp
+  ResourceScriptCppFilter.cpp
   ResourceScriptParser.cpp
   ResourceScriptStmt.cpp
   ResourceScriptToken.cpp
   )

llvm/trunk/tools/llvm-rc/ResourceScriptCppFilter.h

+//===-- ResourceScriptCppFilter.h ------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===---------------------------------------------------------------------===//
+//
+// This filters the input to llvm-rc for preprocessor markers, removing
+// preprocessing directives that a preprocessor can output or leave behind.
+//
+// It also filters out any contribution from files named *.h or *.c, based
+// on preprocessor line markers. When preprocessing RC files, the included
+// headers can leave behind C declarations, that RC doesn't understand.
+// Rc.exe simply discards anything that comes from files named *.h or *.h.
+//
+// https://msdn.microsoft.com/en-us/library/windows/desktop/aa381033(v=vs.85).aspx
+//
+//===---------------------------------------------------------------------===//
+
+#ifndef LLVM_TOOLS_LLVMRC_RESOURCESCRIPTCPPFILTER_H
+#define LLVM_TOOLS_LLVMRC_RESOURCESCRIPTCPPFILTER_H
+
+#include "llvm/ADT/StringRef.h"
+
+#include <string>
+
+namespace llvm {
+
+std::string filterCppOutput(StringRef Input);
+
+} // namespace llvm
+
+#endif

llvm/trunk/tools/llvm-rc/ResourceScriptCppFilter.cpp

+//===-- ResourceScriptCppFilter.cpp ----------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===---------------------------------------------------------------------===//
+//
+// This file implements an interface defined in ResourceScriptCppFilter.h.
+//
+//===---------------------------------------------------------------------===//
+
+#include "ResourceScriptCppFilter.h"
+#include "llvm/ADT/StringExtras.h"
+
+#include <vector>
+
+using namespace llvm;
+
+namespace {
+
+class Filter {
+public:
+  explicit Filter(StringRef Input) : Data(Input), DataLength(Input.size()) {}
+
+  std::string run();
+
+private:
+  // Parse the line, returning whether the line should be included in
+  // the output.
+  bool parseLine(StringRef Line);
+
+  bool streamEof() const;
+
+  StringRef Data;
+  size_t DataLength;
+
+  size_t Pos = 0;
+  bool Outputting = true;
+};
+
+std::string Filter::run() {
+  std::vector<StringRef> Output;
+
+  while (!streamEof() && Pos != StringRef::npos) {
+    size_t LineStart = Pos;
+    Pos = Data.find_first_of("\r\n", Pos);
+    Pos = Data.find_first_not_of("\r\n", Pos);
+    StringRef Line = Data.take_front(Pos).drop_front(LineStart);
+
+    if (parseLine(Line))
+      Output.push_back(Line);
+  }
+
+  return llvm::join(Output, "");
+}
+
+bool Filter::parseLine(StringRef Line) {
+  Line = Line.ltrim();
+
+  if (!Line.consume_front("#")) {
+    // A normal content line, filtered according to the current mode.
+    return Outputting;
+  }
+
+  // Found a preprocessing directive line. From here on, we always return
+  // false since the preprocessing directives should be filtered out.
+
+  Line.consume_front("line");
+  if (!Line.startswith(" "))
+    return false; // Not a line directive (pragma etc).
+
+  // #line 123 "path/file.h"
+  // # 123 "path/file.h" 1
+
+  Line =
+      Line.ltrim(); // There could be multiple spaces after the #line directive
+
+  size_t N;
+  if (Line.consumeInteger(10, N)) // Returns true to signify an error
+    return false;
+
+  Line = Line.ltrim();
+
+  if (!Line.consume_front("\""))
+    return false; // Malformed line, no quote found.
+
+  // Split the string at the last quote (in case the path name had
+  // escaped quotes as well).
+  Line = Line.rsplit('"').first;
+
+  StringRef Ext = Line.rsplit('.').second;
+
+  if (Ext.equals_lower("h") || Ext.equals_lower("c")) {
+    Outputting = false;
+  } else {
+    Outputting = true;
+  }
+
+  return false;
+}
+
+bool Filter::streamEof() const { return Pos == DataLength; }
+
+} // anonymous namespace
+
+namespace llvm {
+
+std::string filterCppOutput(StringRef Input) { return Filter(Input).run(); }
+
+} // namespace llvm

llvm/trunk/tools/llvm-rc/llvm-rc.cpp

 //===-- llvm-rc.cpp - Compile .rc scripts into .res -------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // Compile .rc scripts into .res files. This is intended to be a
 // platform-independent port of Microsoft's rc.exe tool.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ResourceFileWriter.h"
+#include "ResourceScriptCppFilter.h"
 #include "ResourceScriptParser.h"
 #include "ResourceScriptStmt.h"
 #include "ResourceScriptToken.h"
 
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
int main(int Argc, const char **Argv) {
   if (!File) {
     fatalError("Error opening file '" + Twine(InArgsInfo[0]) +
                "': " + File.getError().message());
   }
 
   std::unique_ptr<MemoryBuffer> FileContents = std::move(*File);
   StringRef Contents = FileContents->getBuffer();
 
-  std::vector<RCToken> Tokens = ExitOnErr(tokenizeRC(Contents));
+  std::string FilteredContents = filterCppOutput(Contents);
+  std::vector<RCToken> Tokens = ExitOnErr(tokenizeRC(FilteredContents));
 
   if (BeVerbose) {
     const Twine TokenNames[] = {
 #define TOKEN(Name) #Name,
 #define SHORT_TOKEN(Name, Ch) #Name,
 #include "ResourceScriptTokenList.def"
     };