[llvm-rc] Add support for all missing dialog controls

[zturner]

Looks good. AFAICT we're now only missing PUSHBOX and CONTROL. If these don't present any additional difficulty, maybe better to just add them now for the sake of completeness?

[mstorsjo]

In D46507#1089961, @zturner wrote:

Looks good. AFAICT we're now only missing PUSHBOX and CONTROL. If these don't present any additional difficulty, maybe better to just add them now for the sake of completeness?

Sure, I'll have another look at them.

[amccarth]

In D46507#1089961, @zturner wrote:

Looks good. AFAICT we're now only missing PUSHBOX and CONTROL. If these don't present any additional difficulty, maybe better to just add them now for the sake of completeness?

CONTROL is important. I'd never heard of PUSHBOX. Interesting.

[mstorsjo]

Added PUSHBOX and CONTROL - now this should be complete.

[zturner]

Lgtm

[amccarth]

tools/llvm-rc/ResourceScriptParser.cpp

484 ↗

(On Diff #145632)

This is OK for now, but in the future I think you'll need special code for style and exstyle because those are really expressions that don't work exactly like regular C-style bitwise operations.

So I'm not sure how much readIntsWithCommas is buying versus reading the arguments into well-named variables like X, Y, Width, Height, etc., which would then make reading the Control c'tor calls a lot easier and would highlight the difference between the CONTROL syntax versus the syntax for the pre-defined control types.

About a third of this function is special case handling for CONTROL when the only differences are two little tweaks to the syntax (position of style and the explicit class parameter). It feels like a lot of near-duplicate code (e.g., the TakeOptArg lambda has to be defined twice) to handle the tweaks.

[amccarth]

tools/llvm-rc/ResourceScriptStmt.cpp

146 ↗

(On Diff #145632)

For CONTROL, where does the 0x2A come from? I know 0x5000000 is WS_CHILD | WS_VISIBLE.

It would be nice to see the styles represented in a way that makes it easier to check the table.

[mstorsjo]

tools/llvm-rc/ResourceScriptStmt.cpp

146 ↗

(On Diff #145632)

Oh, that 0x2A must be from the test file itself. Will fix.

[mstorsjo]

tools/llvm-rc/ResourceScriptParser.cpp

484 ↗

(On Diff #145632)

I rewrote this, making it just one main function with minor exceptions for CONTROL vs others, making the similarities more visible, hopefully like you wanted.

tools/llvm-rc/ResourceScriptStmt.cpp

146 ↗

(On Diff #145632)

I'll leave splitting up the style hardcoded values into symbolic values to a separate patch, since it's already that way.

[mstorsjo]

Unified handling of CONTROL vs other controls, removed stray extra values in the CONTROL style base value.

[amccarth]

Thanks!

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

Types DIALOGEX 12345, -11215, 0x1234, 0x1EED, 0x51525354 {
   RTEXT "R", 11, 12, 13, 14, 15
 
   PUSHBUTTON "PB", 1001, 1002, 1003, 1004, 1005
   DEFPUSHBUTTON "DPB", 1006, 1007, 1008, 1009, 1010
 
   EDITTEXT 2001, 2002, 2003, 2004, 2005
 
   LTEXT 65535, 3001, 3002, 3003, 3004, 3005
+
+  AUTO3STATE "A3", 12, 2, 3, 4, 5
+  AUTOCHECKBOX "AC", 13, 2, 3, 4, 5
+  AUTORADIOBUTTON "AR", 14, 2, 3, 4, 5
+  CHECKBOX "C", 15, 2, 3, 4, 5
+  COMBOBOX 16, 2, 3, 4, 5
+  GROUPBOX "G", 17, 2, 3, 4, 5
+  ICON "I", 18, 2, 3, 4, 5
+  LISTBOX 19, 2, 3, 4, 5
+  RADIOBUTTON "R", 20, 2, 3, 4, 5
+  COMBOBOX 21, 2, 3, 4, 5
+  SCROLLBAR 22, 2, 3, 4, 5
+  STATE3 "S", 23, 2, 3, 4, 5
+  PUSHBOX "P", 24, 2, 3, 4, 5
+  CONTROL "C", 25, "Class", 42, 2, 3, 4, 5
 }
 
 EmptyOld DIALOG 1, 2, 3, 4 {}
 
 ArgsOld DIALOG 1, 2, 3, 4 {
   LTEXT "L", 1, 2, 3, 4, 5
   LTEXT "L2", 6, 7, 8, 9, 10, 11
   LTEXT "L3", 12, 13, 14, 15, 16, 17, 18
 
   EDITTEXT 19, 20, 21, 22, 23
   EDITTEXT 24, 25, 26, 27, 28, 29
   EDITTEXT 30, 31, 32, 33, 34, 35, 36
 }
 

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

 ; DIALOG-DAG: Resource type (int): 5
 ; DIALOG-NEXT: Resource name (string): TYPES
 ; DIALOG-NEXT: Data version: 0
 ; DIALOG-NEXT: Memory flags: 0x1030
 ; DIALOG-NEXT: Language ID: 1033
 ; DIALOG-NEXT: Version (major): 0
 ; DIALOG-NEXT: Version (minor): 0
 ; DIALOG-NEXT: Characteristics: 0
-; DIALOG-NEXT: Data size: 282
+; DIALOG-NEXT: Data size: 778
 ; DIALOG-NEXT: Data: (
 ; DIALOG-NEXT:   0000: 0100FFFF 54535251 00000000 00008880  |....TSRQ........|
-; DIALOG-NEXT:   0010: 07003930 31D43412 ED1E0000 00000000  |..901.4.........|
+; DIALOG-NEXT:   0010: 15003930 31D43412 ED1E0000 00000000  |..901.4.........|
 ; DIALOG-NEXT:   0020: 00000000 00000000 00000250 02000300  |...........P....|
 ; DIALOG-NEXT:   0030: 04000500 01000000 FFFF8200 4C000000  |............L...|
 ; DIALOG-NEXT:   0040: 00000000 00000000 00000000 01000250  |...............P|
 ; DIALOG-NEXT:   0050: 07000800 09000A00 06000000 FFFF8200  |................|
 ; DIALOG-NEXT:   0060: 43000000 00000000 00000000 00000000  |C...............|
 ; DIALOG-NEXT:   0070: 02000250 0C000D00 0E000F00 0B000000  |...P............|
 ; DIALOG-NEXT:   0080: FFFF8200 52000000 00000000 00000000  |....R...........|
 ; DIALOG-NEXT:   0090: 00000000 00000150 EA03EB03 EC03ED03  |.......P........|
 ; DIALOG-NEXT:   00A0: E9030000 FFFF8000 50004200 00000000  |........P.B.....|
 ; DIALOG-NEXT:   00B0: 00000000 00000000 01000150 EF03F003  |...........P....|
 ; DIALOG-NEXT:   00C0: F103F203 EE030000 FFFF8000 44005000  |............D.P.|
 ; DIALOG-NEXT:   00D0: 42000000 00000000 00000000 00000000  |B...............|
 ; DIALOG-NEXT:   00E0: 00008150 D207D307 D407D507 D1070000  |...P............|
 ; DIALOG-NEXT:   00F0: FFFF8100 00000000 00000000 00000000  |................|
 ; DIALOG-NEXT:   0100: 00000250 BA0BBB0B BC0BBD0B B90B0000  |...P............|
-; DIALOG-NEXT:   0110: FFFF8200 FFFFFFFF 0000               |..........|
+; DIALOG-NEXT:   0110: FFFF8200 FFFFFFFF 00000000 00000000  |................|
+; DIALOG-NEXT:   0120: 00000000 06000150 02000300 04000500  |.......P........|
+; DIALOG-NEXT:   0130: 0C000000 FFFF8000 41003300 00000000  |........A.3.....|
+; DIALOG-NEXT:   0140: 00000000 00000000 03000150 02000300  |...........P....|
+; DIALOG-NEXT:   0150: 04000500 0D000000 FFFF8000 41004300  |............A.C.|
+; DIALOG-NEXT:   0160: 00000000 00000000 00000000 09000050  |...............P|
+; DIALOG-NEXT:   0170: 02000300 04000500 0E000000 FFFF8000  |................|
+; DIALOG-NEXT:   0180: 41005200 00000000 00000000 00000000  |A.R.............|
+; DIALOG-NEXT:   0190: 02000150 02000300 04000500 0F000000  |...P............|
+; DIALOG-NEXT:   01A0: FFFF8000 43000000 00000000 00000000  |....C...........|
+; DIALOG-NEXT:   01B0: 00000000 00000050 02000300 04000500  |.......P........|
+; DIALOG-NEXT:   01C0: 10000000 FFFF8500 00000000 00000000  |................|
+; DIALOG-NEXT:   01D0: 00000000 07000050 02000300 04000500  |.......P........|
+; DIALOG-NEXT:   01E0: 11000000 FFFF8000 47000000 00000000  |........G.......|
+; DIALOG-NEXT:   01F0: 00000000 00000000 03000050 02000300  |...........P....|
+; DIALOG-NEXT:   0200: 04000500 12000000 FFFF8200 49000000  |............I...|
+; DIALOG-NEXT:   0210: 00000000 00000000 00000000 01008050  |...............P|
+; DIALOG-NEXT:   0220: 02000300 04000500 13000000 FFFF8300  |................|
+; DIALOG-NEXT:   0230: 00000000 00000000 00000000 04000050  |...............P|
+; DIALOG-NEXT:   0240: 02000300 04000500 14000000 FFFF8000  |................|
+; DIALOG-NEXT:   0250: 52000000 00000000 00000000 00000000  |R...............|
+; DIALOG-NEXT:   0260: 00000050 02000300 04000500 15000000  |...P............|
+; DIALOG-NEXT:   0270: FFFF8500 00000000 00000000 00000000  |................|
+; DIALOG-NEXT:   0280: 00000050 02000300 04000500 16000000  |...P............|
+; DIALOG-NEXT:   0290: FFFF8400 00000000 00000000 00000000  |................|
+; DIALOG-NEXT:   02A0: 05000150 02000300 04000500 17000000  |...P............|
+; DIALOG-NEXT:   02B0: FFFF8000 53000000 00000000 00000000  |....S...........|
+; DIALOG-NEXT:   02C0: 00000000 0A000150 02000300 04000500  |.......P........|
+; DIALOG-NEXT:   02D0: 18000000 FFFF8000 50000000 00000000  |........P.......|
+; DIALOG-NEXT:   02E0: 00000000 00000000 2A000050 02000300  |........*..P....|
+; DIALOG-NEXT:   02F0: 04000500 19000000 43006C00 61007300  |........C.l.a.s.|
+; DIALOG-NEXT:   0300: 73000000 43000000 0000               |s...C.....|
 ; DIALOG-NEXT: )
 
 ; DIALOG-DAG: Resource type (int): 5
 ; DIALOG-NEXT: Resource name (string): EMPTYOLD
 ; DIALOG-NEXT: Data version: 0
 ; DIALOG-NEXT: Memory flags: 0x1030
 ; DIALOG-NEXT: Language ID: 1033
 ; DIALOG-NEXT: Version (major): 0

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

if (!IsExtended) {
     RETURN_IF_ERROR(checkNumberFits<uint16_t>(
         Ctl.ID, "Control ID in simple DIALOG resource"));
     writeInt<uint16_t>(Ctl.ID);
   } else {
     writeInt<uint32_t>(Ctl.ID);
   }
 
   // Window class - either 0xFFFF + 16-bit integer or a string.
-  RETURN_IF_ERROR(writeIntOrString(IntOrString(TypeInfo.CtlClass)));
+  RETURN_IF_ERROR(writeIntOrString(Ctl.Class));
 
   // Element caption/reference ID. ID is preceded by 0xFFFF.
   RETURN_IF_ERROR(checkIntOrString(Ctl.Title, "Control reference ID"));
   RETURN_IF_ERROR(writeIntOrString(Ctl.Title));
 
   // # bytes of extra creation data count. Don't pass any.
   writeInt<uint16_t>(0);
 

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

Expected<Control> RCParser::parseControl() {
   // Read caption if necessary.
   IntOrString Caption{StringRef()};
   if (CtlInfo->getValue().HasTitle) {
     ASSIGN_OR_RETURN(CaptionResult, readIntOrString());
     RETURN_IF_ERROR(consumeType(Kind::Comma));
     Caption = *CaptionResult;
   }
 
-  ASSIGN_OR_RETURN(Args, readIntsWithCommas(5, 8));
+  ASSIGN_OR_RETURN(ID, readInt());
+  RETURN_IF_ERROR(consumeType(Kind::Comma));
 
-  auto TakeOptArg = [&Args](size_t Id) -> Optional<uint32_t> {
-    return Args->size() > Id ? (uint32_t)(*Args)[Id] : Optional<uint32_t>();
-  };
+  IntOrString Class;
+  Optional<uint32_t> Style;
+  if (ClassUpper == "CONTROL") {
+    // CONTROL text, id, class, style, x, y, width, height [, exstyle] [, helpID]
+    ASSIGN_OR_RETURN(ClassStr, readString());
+    RETURN_IF_ERROR(consumeType(Kind::Comma));
+    Class = *ClassStr;
+    ASSIGN_OR_RETURN(StyleVal, readInt());
+    RETURN_IF_ERROR(consumeType(Kind::Comma));
+    Style = *StyleVal;
+  } else {
+    Class = CtlInfo->getValue().CtlClass;
+  }
+
+  // x, y, width, height
+  ASSIGN_OR_RETURN(Args, readIntsWithCommas(4, 4));
+
+  if (ClassUpper != "CONTROL") {
+    if (consumeOptionalType(Kind::Comma)) {
+      ASSIGN_OR_RETURN(Val, readInt());
+      Style = *Val;
+    }
+  }
+
+  Optional<uint32_t> ExStyle;
+  if (consumeOptionalType(Kind::Comma)) {
+    ASSIGN_OR_RETURN(Val, readInt());
+    ExStyle = *Val;
+  }
+  Optional<uint32_t> HelpID;
+  if (consumeOptionalType(Kind::Comma)) {
+    ASSIGN_OR_RETURN(Val, readInt());
+    HelpID = *Val;
+  }
 
-  return Control(*ClassResult, Caption, (*Args)[0], (*Args)[1], (*Args)[2],
-                 (*Args)[3], (*Args)[4], TakeOptArg(5), TakeOptArg(6),
-                 TakeOptArg(7));
+  return Control(*ClassResult, Caption, *ID, (*Args)[0], (*Args)[1],
+                 (*Args)[2], (*Args)[3], Style, ExStyle, HelpID, Class);
 }
 
 RCParser::ParseType RCParser::parseBitmapResource() {
   ASSIGN_OR_RETURN(Arg, readFilename());
   return llvm::make_unique<BitmapResource>(*Arg);
 }
 
 RCParser::ParseType RCParser::parseIconResource() {

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

 
 // Single control definition.
 class Control {
 public:
   StringRef Type;
   IntOrString Title;
   uint32_t ID, X, Y, Width, Height;
   Optional<uint32_t> Style, ExtStyle, HelpID;
+  IntOrString Class;
 
   // Control classes as described in DLGITEMTEMPLATEEX documentation.
   //
   // Ref: msdn.microsoft.com/en-us/library/windows/desktop/ms645389.aspx
   enum CtlClasses {
     ClsButton = 0x80,
     ClsEdit = 0x81,
     ClsStatic = 0x82,
     ClsListBox = 0x83,
     ClsScrollBar = 0x84,
     ClsComboBox = 0x85
   };
 
   // Simple information about a single control type.
   struct CtlInfo {
     uint32_t Style;
     uint16_t CtlClass;
     bool HasTitle;
   };
 
   Control(StringRef CtlType, IntOrString CtlTitle, uint32_t CtlID,
           uint32_t PosX, uint32_t PosY, uint32_t ItemWidth, uint32_t ItemHeight,
           Optional<uint32_t> ItemStyle, Optional<uint32_t> ExtItemStyle,
-          Optional<uint32_t> CtlHelpID)
+          Optional<uint32_t> CtlHelpID, IntOrString CtlClass)
       : Type(CtlType), Title(CtlTitle), ID(CtlID), X(PosX), Y(PosY),
         Width(ItemWidth), Height(ItemHeight), Style(ItemStyle),
-        ExtStyle(ExtItemStyle), HelpID(CtlHelpID) {}
+        ExtStyle(ExtItemStyle), HelpID(CtlHelpID), Class(CtlClass) {}
 
   static const StringMap<CtlInfo> SupportedCtls;
 
   raw_ostream &log(raw_ostream &) const;
 };
 
 // Single dialog definition. We don't create distinct classes for DIALOG and
 // DIALOGEX because of their being too similar to each other. We only have a

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

for (const auto &String : Table)
     OS << "  " << String.first << " => " << String.second << "\n";
   return OS;
 }
 
 const StringMap<Control::CtlInfo> Control::SupportedCtls = {
     {"LTEXT", CtlInfo{0x50020000, ClsStatic, true}},
     {"CTEXT", CtlInfo{0x50020001, ClsStatic, true}},
     {"RTEXT", CtlInfo{0x50020002, ClsStatic, true}},
+    {"ICON", CtlInfo{0x50000003, ClsStatic, true}},
     {"PUSHBUTTON", CtlInfo{0x50010000, ClsButton, true}},
     {"DEFPUSHBUTTON", CtlInfo{0x50010001, ClsButton, true}},
+    {"AUTO3STATE", CtlInfo{0x50010006, ClsButton, true}},
+    {"AUTOCHECKBOX", CtlInfo{0x50010003, ClsButton, true}},
+    {"AUTORADIOBUTTON", CtlInfo{0x50000009, ClsButton, true}},
+    {"CHECKBOX", CtlInfo{0x50010002, ClsButton, true}},
+    {"GROUPBOX", CtlInfo{0x50000007, ClsButton, true}},
+    {"RADIOBUTTON", CtlInfo{0x50000004, ClsButton, true}},
+    {"STATE3", CtlInfo{0x50010005, ClsButton, true}},
+    {"PUSHBOX", CtlInfo{0x5001000A, ClsButton, true}},
     {"EDITTEXT", CtlInfo{0x50810000, ClsEdit, false}},
+    {"COMBOBOX", CtlInfo{0x50000000, ClsComboBox, false}},
+    {"LISTBOX", CtlInfo{0x50800001, ClsListBox, false}},
+    {"SCROLLBAR", CtlInfo{0x50000000, ClsScrollBar, false}},
+    {"CONTROL", CtlInfo{0x50000000, 0, true}},
 };
 
 raw_ostream &Control::log(raw_ostream &OS) const {
   OS << "  Control (" << ID << "): " << Type << ", title: " << Title
      << ", loc: (" << X << ", " << Y << "), size: [" << Width << ", " << Height
      << "]";
   if (Style)
     OS << ", style: " << *Style;