1648346 - const unsigned long values of 0x80000000 in XPIDL might be interpreted as signed integers on Windows

Status: NEW

(Core :: XPCOM, defect)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

I am not entirely sure what's going on here, so I'll explain how this was found.

1. In bug 1645896, we added a case to a switch block[1] that was comparing a uint32_t value to various nsIWebProgressListener state constants[2], which are all defined as const unsigned long.
2. The newly added case was for a state constant with value 0x80000000.
3. This caused build failure on Windows, with the compiler complaining error: case value evaluates to -2147483648, which cannot be narrowed to type 'uint32_t' (aka 'unsigned int') [-Wc++11-narrowing] [3]
4. It appears that the internal type of the enum generated by the XPIDL codegen does not end up being an unsigned int type on Windows, though the literals are suffixed with "U"[4] at least on whatever platform searchfox indexes generated files for.

I'm quite terrible at C++ so I'm not sure if this is a bug but it sure sounds like one. Should the XPIDL codegen be emitting : unsigned long on the enum declaration? There are three other instances of const unsigned long values of 0x80000000 in m-c at the time of writing this[5] but they seem unused, so maybe that's why this hasn't come up before.

FWIW, if indeed this is a bug, I'd be interested in working on this (as a learning exercise) if the fix isn't too complex.

[1] https://phabricator.services.mozilla.com/D80675
[2] https://searchfox.org/mozilla-central/rev/a87a1c3b543475276e6d57a7a80cb02f3e42b6ed/uriloader/base/nsIWebProgressListener.idl#336-354
[3] https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=307352680&repo=autoland&lineNumber=33354
[4] https://searchfox.org/mozilla-central/source/__GENERATED__/dist/include/nsIWebProgressListener.h#77
[5] https://searchfox.org/mozilla-central/search?q=0x80000000&path=.idl

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

From a bit of research, I think the issue might be that the C++ standard only specifies that an enum's underlying type, if not explicitly specified, should be any integer type wide enough to hold the largest value. I guess that could mean that the underlying type could end up being a signed 32 bit int since it's technically wide enough, even though the literal is suffixed. If this is right, it's likely that emitting an explicit type for the enum will fix the issue.

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Nihanth Subramanya [:nhnt11] from comment #1)

From a bit of research, I think the issue might be that the C++ standard only specifies that an enum's underlying type, if not explicitly specified, should be any integer type wide enough to hold the largest value. I guess that could mean that the underlying type could end up being a signed 32 bit int since it's technically wide enough, even though the literal is suffixed. If this is right, it's likely that emitting an explicit type for the enum will fix the issue.

Ah, but maybe it's not trivial to do this if e.g. the IDL defines a combination of signed and unsigned values. Maybe an enum for each underlying type? i.e. if the IDL defines long, unsigned long, and unsigned int constants, we'd end up with three separate enums. 🤷‍♂️

edit: Thought about this a bit more and I do think that if we make it appear that one can control the type of a constant in the IDL syntax, then the generated code should preserve those semantics, so it's worth doing the enum-per-type approach. Otherwise we should not allow explicit typing on consts in XPIDL, since that information gets lost anyway. Still thinking about this.

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1648346 - [Proof of concept] emit two separate enums for signed and unsigned consts in idl.

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d4771261fcfba9236f14094c3380399e9d7ecc6

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Nihanth Subramanya [:nhnt11] from comment #4)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d4771261fcfba9236f14094c3380399e9d7ecc6

Indeed, this proof-of-concept patch fixes the build failure I mentioned in comment 0 on Windows, but reveals that we have several signed/unsigned int comparisons that previously were uncaught because the underlying type of the enums was signed, even though the constants were explicitly declared unsigned. So I guess this will be two patches: one to update idl-parser and one to fix all the instances of signed-unsigned int comparisons.

Comment 6

[(inactive) Nihanth Subramanya [:nhnt11]]

I'm not going to pick this back up in the near future.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Adding bug 239460 as a see-also, as we considered switching the compiler to use proper constants instead of enums, but it caused issues on Windows, due to MSVC's odd handling of const int X = 0; as being coerceable to pointer types, due to having an integer value of 0. If we wanted to do better here, we might end up needing to continue to use enums for 0 constants or something like that, which would be quite unfortunate.