Page MenuHomePhabricator

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

Authored by mstorsjo on Mar 27 2020, 1:05 PM.

Details

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...

Diff Detail

Event Timeline

mstorsjo created this revision.Mar 27 2020, 1:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2020, 1:05 PM
rnk accepted this revision.Mar 27 2020, 1:21 PM

lgtm

This revision is now accepted and ready to land.Mar 27 2020, 1:21 PM
amccarth accepted this revision.Mar 27 2020, 2:40 PM

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.

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.

This revision was automatically updated to reflect the committed changes.
rnk added a comment.Mar 28 2020, 9:20 AM

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.