[llvm-rc] Support EXSTYLE statement.

[phosek]

LGTM although you may want to get a review from someone who's more familiar with llvm-rc.

tools/llvm-rc/ResourceScriptStmt.h

872 ↗

(On Diff #175753)

Nit: I'd consider naming it just ExStyle to match the name of the statement precisely.

[mstorsjo]

Looks great in general, but I think I agree that having it named ExStyle would be less prone to confusion.

tools/llvm-rc/ResourceVisitor.h

53 ↗

(On Diff #175753)

These were alpabetically ordered before. The same goes for a few other places in the patch as well.

[jacek]

Thanks for reviews, I addressed comments.

FWIW, I used ExtStyle instead of ExStyle in previous version because that's what's used in a few places in sources already, but I agree that ExStyle fits better.

[mstorsjo]

LGTM

However, I can't commit it as is, as the diff for test/tools/llvm-rc/Inputs/tag-dialog-headers.rc is missing, as it's treated as binary. I'm not sure exactly why we have that there, but you can e.g. remove the relevant line from .gitattributes before diffing, to get that file included in the diff.

tools/llvm-rc/ResourceVisitor.h

30 ↗

(On Diff #175846)

This one is still unordered, but I could change that before committing.

llvm/trunk/test/tools/llvm-rc/Inputs/tag-dialog-headers.rc

 24 DIALOGEX 5, 5, 5, 5 CAPTION "" STYLE 0 {}
 25 DIALOG 5, 5, 5, 5 CAPTION "" STYLE 0 {}
 
 26 DIALOGEX 1, 2, 3, 4, 5 {}
 
 27 DIALOGEX 5, 5, 5, 5 CLASS "Foobar" {}
 28 DIALOG 5, 5, 5, 5 CLASS 42 {}
 
+29 DIALOGEX 0, 1, 0, 9 EXSTYLE 0x01234567 {}
+30 DIALOG 0, 1, 0, 9 EXSTYLE 0x01234567 {}
+
+31 DIALOGEX 0, 1, 0, 9 STYLE 0x76543210 EXSTYLE 0x01234567 {}
+32 DIALOG 0, 1, 0, 9 EXSTYLE 0x0123456 STYLE 0x76543210 {}

llvm/trunk/test/tools/llvm-rc/tag-dialog.test

 ; HEADERS-NEXT: Version (minor): 0
 ; HEADERS-NEXT: Characteristics: 0
 ; HEADERS-NEXT: Data size: 26
 ; HEADERS-NEXT: Data: (
 ; HEADERS-NEXT:   0000: 00008880 00000000 00000500 05000500  |................|
 ; HEADERS-NEXT:   0010: 05000000 FFFF2A00 0000               |......*...|
 ; HEADERS-NEXT: )
 
+; HEADERS-DAG: Resource type (int): 5
+; HEADERS-NEXT: Resource name (int): 29
+; HEADERS-NEXT: Data version: 0
+; HEADERS-NEXT: Memory flags: 0x1030
+; HEADERS-NEXT: Language ID: 1033
+; HEADERS-NEXT: Version (major): 0
+; HEADERS-NEXT: Version (minor): 0
+; HEADERS-NEXT: Characteristics: 0
+; HEADERS-NEXT: Data size: 32
+; HEADERS-NEXT: Data: (
+; HEADERS-NEXT:   0000: 0100FFFF 00000000 67452301 00008880  |........gE#.....|
+; HEADERS-NEXT:   0010: 00000000 01000000 09000000 00000000  |................|
+; HEADERS-NEXT: )
+
+; HEADERS-DAG: Resource type (int): 5
+; HEADERS-NEXT: Resource name (int): 30
+; HEADERS-NEXT: Data version: 0
+; HEADERS-NEXT: Memory flags: 0x1030
+; HEADERS-NEXT: Language ID: 1033
+; HEADERS-NEXT: Version (major): 0
+; HEADERS-NEXT: Version (minor): 0
+; HEADERS-NEXT: Characteristics: 0
+; HEADERS-NEXT: Data size: 24
+; HEADERS-NEXT: Data: (
+; HEADERS-NEXT:   0000: 00008880 67452301 00000000 01000000  |....gE#.........|
+; HEADERS-NEXT:   0010: 09000000 00000000                    |........|
+; HEADERS-NEXT: )
+
+; HEADERS-DAG: Resource type (int): 5
+; HEADERS-NEXT: Resource name (int): 31
+; HEADERS-NEXT: Data version: 0
+; HEADERS-NEXT: Memory flags: 0x1030
+; HEADERS-NEXT: Language ID: 1033
+; HEADERS-NEXT: Version (major): 0
+; HEADERS-NEXT: Version (minor): 0
+; HEADERS-NEXT: Characteristics: 0
+; HEADERS-NEXT: Data size: 32
+; HEADERS-NEXT: Data: (
+; HEADERS-NEXT:   0000: 0100FFFF 00000000 67452301 10325476  |........gE#..2Tv|
+; HEADERS-NEXT:   0010: 00000000 01000000 09000000 00000000  |................|
+; HEADERS-NEXT: )
+
+; HEADERS-DAG: Resource type (int): 5
+; HEADERS-NEXT: Resource name (int): 32
+; HEADERS-NEXT: Data version: 0
+; HEADERS-NEXT: Memory flags: 0x1030
+; HEADERS-NEXT: Language ID: 1033
+; HEADERS-NEXT: Version (major): 0
+; HEADERS-NEXT: Version (minor): 0
+; HEADERS-NEXT: Characteristics: 0
+; HEADERS-NEXT: Data size: 24
+; HEADERS-NEXT: Data: (
+; HEADERS-NEXT:   0000: 10325476 56341200 00000000 01000000  |.2TvV4..........|
+; HEADERS-NEXT:   0010: 09000000 00000000                    |........|
+; HEADERS-NEXT: )
+
 
 ; RUN: not llvm-rc /FO %t %p/Inputs/tag-dialog-large-coord.rc 2>&1 | FileCheck %s --check-prefix COORD1
 
 ; COORD1: llvm-rc: Error in DIALOGEX statement (ID 1):
 ; COORD1-NEXT: Dialog x-coordinate (50000) does not fit in 16-bit signed integer type.
 
 
 ; RUN: not llvm-rc /FO %t %p/Inputs/tag-dialog-large-coord-neg.rc 2>&1 | FileCheck %s --check-prefix COORD2

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

public:
   Error visitMenuResource(const RCResource *) override;
   Error visitVersionInfoResource(const RCResource *) override;
   Error visitStringTableResource(const RCResource *) override;
   Error visitUserDefinedResource(const RCResource *) override;
 
   Error visitCaptionStmt(const CaptionStmt *) override;
   Error visitCharacteristicsStmt(const CharacteristicsStmt *) override;
   Error visitClassStmt(const ClassStmt *) override;
+  Error visitExStyleStmt(const ExStyleStmt *) override;
   Error visitFontStmt(const FontStmt *) override;
   Error visitLanguageStmt(const LanguageResource *) override;
   Error visitStyleStmt(const StyleStmt *) override;
   Error visitVersionStmt(const VersionStmt *) override;
 
   // Stringtables are output at the end of .res file. We need a separate
   // function to do it.
   Error dumpAllStringTables();
 
   bool AppendNull; // Append '\0' to each existing STRINGTABLE element?
 
   struct ObjectInfo {
     uint16_t LanguageInfo;
     uint32_t Characteristics;
     uint32_t VersionInfo;
 
     Optional<uint32_t> Style;
+    Optional<uint32_t> ExStyle;
     StringRef Caption;
     struct FontInfo {
       uint32_t Size;
       StringRef Typeface;
       uint32_t Weight;
       bool IsItalic;
       uint32_t Charset;
     };

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

 }
 
 Error ResourceFileWriter::visitCharacteristicsStmt(
     const CharacteristicsStmt *Stmt) {
   ObjectData.Characteristics = Stmt->Value;
   return Error::success();
 }
 
+Error ResourceFileWriter::visitExStyleStmt(const ExStyleStmt *Stmt) {
+  ObjectData.ExStyle = Stmt->Value;
+  return Error::success();
+}
+
 Error ResourceFileWriter::visitFontStmt(const FontStmt *Stmt) {
   RETURN_IF_ERROR(checkNumberFits<uint16_t>(Stmt->Size, "Font size"));
   RETURN_IF_ERROR(checkNumberFits<uint16_t>(Stmt->Weight, "Font weight"));
   RETURN_IF_ERROR(checkNumberFits<uint8_t>(Stmt->Charset, "Font charset"));
   ObjectInfo::FontInfo Font{Stmt->Size, Stmt->Name, Stmt->Weight, Stmt->Italic,
                             Stmt->Charset};
   ObjectData.Font.emplace(Font);
   return Error::success();
else
     UsedStyle &= ~StyleFontFlag;
 
   // Actually, in case of empty (but existent) caption, the examined field
   // is equal to "\"\"". That's why empty captions are still noticed.
   if (ObjectData.Caption != "")
     UsedStyle |= StyleCaptionFlag;
 
   const uint16_t DialogExMagic = 0xFFFF;
+  uint32_t ExStyle = ObjectData.ExStyle.getValueOr(0);
 
   // Write DIALOG(EX) header prefix. These are pretty different.
   if (!Res->IsExtended) {
     // We cannot let the higher word of DefaultStyle be equal to 0xFFFF.
     // In such a case, whole object (in .res file) is equivalent to a
     // DIALOGEX. It might lead to access violation/segmentation fault in
     // resource readers. For example,
     //   1 DIALOG 0, 0, 0, 65432
     //   STYLE 0xFFFF0001 {}
     // would be compiled to a DIALOGEX with 65432 controls.
     if ((UsedStyle >> 16) == DialogExMagic)
       return createError("16 higher bits of DIALOG resource style cannot be"
                          " equal to 0xFFFF");
 
     struct {
       ulittle32_t Style;
       ulittle32_t ExtStyle;
     } Prefix{ulittle32_t(UsedStyle),
-             ulittle32_t(0)}; // As of now, we don't keep EXSTYLE.
+             ulittle32_t(ExStyle)};
 
     writeObject(Prefix);
   } else {
     struct {
       ulittle16_t Version;
       ulittle16_t Magic;
       ulittle32_t HelpID;
       ulittle32_t ExtStyle;
       ulittle32_t Style;
     } Prefix{ulittle16_t(1), ulittle16_t(DialogExMagic),
-             ulittle32_t(Res->HelpID), ulittle32_t(0), ulittle32_t(UsedStyle)};
+             ulittle32_t(Res->HelpID), ulittle32_t(ExStyle), ulittle32_t(UsedStyle)};
 
     writeObject(Prefix);
   }
 
   // Now, a common part. First, fixed-length fields.
   RETURN_IF_ERROR(checkNumberFits<uint16_t>(Res->Controls.size(),
                                             "Number of dialog controls"));
   RETURN_IF_ERROR(

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

private:
   Expected<VersionInfoResource::VersionInfoFixed> parseVersionInfoFixed();
 
   // Optional statement parsers.
   ParseOptionType parseLanguageStmt();
   ParseOptionType parseCharacteristicsStmt();
   ParseOptionType parseVersionStmt();
   ParseOptionType parseCaptionStmt();
   ParseOptionType parseClassStmt();
+  ParseOptionType parseExStyleStmt();
   ParseOptionType parseFontStmt(OptStmtType DialogType);
   ParseOptionType parseStyleStmt();
 
   // Raises an error. If IsAlreadyRead = false (default), this complains about
   // the token that couldn't be parsed. If the flag is on, this complains about
   // the correctly read token that makes no sense (that is, the current parser
   // state is beyond the erroneous token.)
   Error getExpectedError(const Twine &Message, bool IsAlreadyRead = false);

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

RCParser::parseSingleOptionalStatement(OptStmtType StmtsType) {
   if (TypeToken->equals_lower("VERSION"))
     return parseVersionStmt();
 
   if (StmtsType != OptStmtType::BasicStmt) {
     if (TypeToken->equals_lower("CAPTION"))
       return parseCaptionStmt();
     if (TypeToken->equals_lower("CLASS"))
       return parseClassStmt();
+    if (TypeToken->equals_lower("EXSTYLE"))
+      return parseExStyleStmt();
     if (TypeToken->equals_lower("FONT"))
       return parseFontStmt(StmtsType);
     if (TypeToken->equals_lower("STYLE"))
       return parseStyleStmt();
   }
 
   return getExpectedError("optional statement type, BEGIN or '{'",
                           /* IsAlreadyRead = */ true);
return llvm::make_unique<FontStmt>(*SizeResult, *NameResult, FontWeight,
                                      FontItalic, FontCharset);
 }
 
 RCParser::ParseOptionType RCParser::parseStyleStmt() {
   ASSIGN_OR_RETURN(Arg, readInt());
   return llvm::make_unique<StyleStmt>(*Arg);
 }
 
+RCParser::ParseOptionType RCParser::parseExStyleStmt() {
+  ASSIGN_OR_RETURN(Arg, readInt());
+  return llvm::make_unique<ExStyleStmt>(*Arg);
+}
+
 Error RCParser::getExpectedError(const Twine &Message, bool IsAlreadyRead) {
   return make_error<ParserError>(
       Message, IsAlreadyRead ? std::prev(CurLoc) : CurLoc, End);
 }
 
 } // namespace rc
 } // namespace llvm

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

public:
   uint32_t Value;
 
   StyleStmt(uint32_t Style) : Value(Style) {}
   raw_ostream &log(raw_ostream &) const override;
   Twine getResourceTypeName() const override { return "STYLE"; }
   Error visit(Visitor *V) const override { return V->visitStyleStmt(this); }
 };
 
+// EXSTYLE optional statement.
+//
+// Ref: docs.microsoft.com/en-us/windows/desktop/menurc/exstyle-statement
+class ExStyleStmt : public OptionalStmt {
+public:
+  uint32_t Value;
+
+  ExStyleStmt(uint32_t ExStyle) : Value(ExStyle) {}
+  raw_ostream &log(raw_ostream &) const override;
+  Twine getResourceTypeName() const override { return "EXSTYLE"; }
+  Error visit(Visitor *V) const override { return V->visitExStyleStmt(this); }
+};
+
 // CLASS optional statement.
 //
 // Ref: msdn.microsoft.com/en-us/library/windows/desktop/aa380883(v=vs.85).aspx
 class ClassStmt : public OptionalStmt {
 public:
   IntOrString Value;
 
   ClassStmt(IntOrString Class) : Value(Class) {}

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

if (Italic)
     OS << ", italic";
   return OS << ", charset = " << Charset << "\n";
 }
 
 raw_ostream &StyleStmt::log(raw_ostream &OS) const {
   return OS << "Style: " << Value << "\n";
 }
 
+raw_ostream &ExStyleStmt::log(raw_ostream &OS) const {
+  return OS << "ExStyle: " << Value << "\n";
+}
+
 } // namespace rc
 } // namespace llvm

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

 namespace llvm {
 namespace rc {
 
 class RCResource;
 
 class CaptionStmt;
 class ClassStmt;
 class CharacteristicsStmt;
+class ExStyleStmt;
 class FontStmt;
 class LanguageResource;
 class StyleStmt;
 class VersionStmt;
 
 class Visitor {
 public:
   virtual Error visitNullResource(const RCResource *) = 0;
   virtual Error visitAcceleratorsResource(const RCResource *) = 0;
   virtual Error visitBitmapResource(const RCResource *) = 0;
   virtual Error visitCursorResource(const RCResource *) = 0;
   virtual Error visitDialogResource(const RCResource *) = 0;
   virtual Error visitHTMLResource(const RCResource *) = 0;
   virtual Error visitIconResource(const RCResource *) = 0;
   virtual Error visitMenuResource(const RCResource *) = 0;
   virtual Error visitStringTableResource(const RCResource *) = 0;
   virtual Error visitUserDefinedResource(const RCResource *) = 0;
   virtual Error visitVersionInfoResource(const RCResource *) = 0;
 
   virtual Error visitCaptionStmt(const CaptionStmt *) = 0;
   virtual Error visitClassStmt(const ClassStmt *) = 0;
   virtual Error visitCharacteristicsStmt(const CharacteristicsStmt *) = 0;
+  virtual Error visitExStyleStmt(const ExStyleStmt *) = 0;
   virtual Error visitFontStmt(const FontStmt *) = 0;
   virtual Error visitLanguageStmt(const LanguageResource *) = 0;
   virtual Error visitStyleStmt(const StyleStmt *) = 0;
   virtual Error visitVersionStmt(const VersionStmt *) = 0;
 
   virtual ~Visitor() {}
 };
 
 } // namespace rc
 } // namespace llvm
 
 #endif