[llvm-rc] Allow -1 for menu item IDs

[mstorsjo]

Summary

This seems to be used in some resource files, e.g.
https://github.com/wxWidgets/wxWidgets/blob/f3217573d7240411e7817c9d76d965b2452987a2/include/wx/msw/wx.rc#L28.

MSVC rc.exe and GNU windres both allow any value here, and silently
just truncate to uint16_t range. This just explicitly allows the
-1 value and errors out on others - the same was done for control
IDs in dialogs in c1a67857ba0a6ba558818b589fe7c0fcc8f238ae.

I presume there's other cases where seemingly out of range values are used, where rc.exe and GNU windres just allow them through, while llvm-rc strictly checks the range of all values - but these two are the ones I've run into so far...

[rnk]

lgtm

[amccarth]

Negative values, especially -1, for unneeded IDs is indeed a very things in Windows resource files, not just for menu items (and submenus) but also for decorative controls on dialogs. So this looks fine to me, though we might eventually have to relax the constraints even more.

[mstorsjo]

In D76951#1946998, @amccarth wrote:

Negative values, especially -1, for unneeded IDs is indeed a very things in Windows resource files, not just for menu items (and submenus) but also for decorative controls on dialogs. So this looks fine to me, though we might eventually have to relax the constraints even more.

Indeed - that file has got quite a few of such checkNumberFits<uint16_t>(), and to match rc.exe, one should just lose the whole check, but it's not entirely evident which ones (because some of them probably are good to keep as they are). For this particular case, we even have a testcase checking that positive values out of range are rejected though, while rc.exe silently wraps them. In any case, this patch is at least a small step in the right direction without taking on the full issue.

[rnk]

I was going to say, perhaps we should truncate to signed int16_t and check that the value round trips. This would allow negative values, but ban positive values above ~32K. If we ever see a use of -2, -3, etc, that's the logical next step.

llvm/test/tools/llvm-rc/Inputs/tag-menu.rc

BEGIN
   POPUP "&Only separator" {
     MENUITEM SEPARATOR
   }
   POPUP "O&ther things" {
     MENUITEM "&abcde", 1
     MENUITEM "a&bcde", 2
     MENUITEM "ab&cde", 3
     MENUITEM "abc&de", 4
-    MENUITEM "abcd&e", 5
+    MENUITEM "abcd&e", -1
   }
 END
 

llvm/test/tools/llvm-rc/tag-menu.test

 ; MENU-NEXT:   0010: 20007300 65007000 61007200 61007400  | .s.e.p.a.r.a.t.|
 ; MENU-NEXT:   0020: 6F007200 00008000 00000000 90004F00  |o.r...........O.|
 ; MENU-NEXT:   0030: 26007400 68006500 72002000 74006800  |&.t.h.e.r. .t.h.|
 ; MENU-NEXT:   0040: 69006E00 67007300 00000000 01002600  |i.n.g.s.......&.|
 ; MENU-NEXT:   0050: 61006200 63006400 65000000 00000200  |a.b.c.d.e.......|
 ; MENU-NEXT:   0060: 61002600 62006300 64006500 00000000  |a.&.b.c.d.e.....|
 ; MENU-NEXT:   0070: 03006100 62002600 63006400 65000000  |..a.b.&.c.d.e...|
 ; MENU-NEXT:   0080: 00000400 61006200 63002600 64006500  |....a.b.c.&.d.e.|
-; MENU-NEXT:   0090: 00008000 05006100 62006300 64002600  |......a.b.c.d.&.|
+; MENU-NEXT:   0090: 00008000 FFFF6100 62006300 64002600  |......a.b.c.d.&.|
 ; MENU-NEXT:   00A0: 65000000                             |e...|
 ; MENU-NEXT: )
 
 
 ; RUN: not llvm-rc /FO %t -- %p/Inputs/tag-menu-bad-menuitem-id.rc 2>&1 | FileCheck %s --check-prefix BADID
 
 ; BADID: llvm-rc: Error in MENU statement (ID 1):
 ; BADID-NEXT: MENUITEM action ID (100000) does not fit in 16 bits.

llvm/tools/llvm-rc/ResourceFileWriter.cpp

 
 Error ResourceFileWriter::writeMenuDefinition(
     const std::unique_ptr<MenuDefinition> &Def, uint16_t Flags) {
   assert(Def);
   const MenuDefinition *DefPtr = Def.get();
 
   if (auto *MenuItemPtr = dyn_cast<MenuItem>(DefPtr)) {
     writeInt<uint16_t>(Flags);
+    // Some resource files use -1, i.e. UINT32_MAX, for empty menu items.
+    if (MenuItemPtr->Id != static_cast<uint32_t>(-1))
       RETURN_IF_ERROR(
           checkNumberFits<uint16_t>(MenuItemPtr->Id, "MENUITEM action ID"));
     writeInt<uint16_t>(MenuItemPtr->Id);
     RETURN_IF_ERROR(writeCString(MenuItemPtr->Name));
     return Error::success();
   }
 
   if (isa<MenuSeparator>(DefPtr)) {
     writeInt<uint16_t>(Flags);
     writeInt<uint32_t>(0);