20833 - Build warning caused by bug in xpidl, converting unsigned positive number to negative number

Status: VERIFIED FIXED

(Core :: XPCOM, defect, P3)

Description

[Daniel Bratell]

During a build I get lots of
..\..\..\..\dist\include\nsIFileSpecWithUI.h(45) : warning C4146: unary minus
operator applied to unsigned type, result still unsigned

Looking at the code this is caused by
    enum { eExtraFilter = -2147483648 };

which is generated by xpidl when compiling nsIFileSpecWithUI.idl. The source
line in that file is

const unsigned long eExtraFilter = 1<<31;

Apparently a "unsigned long 1<<31" is converted to "-2147483648" which surely is
wrong. I don't know if this causes a bug when using the file. If I get the bit
arithmetic right "-2147483648" and "2147483648" is the same bit pattern when
using two complement, but we might not always be so lucky.

I didn't know what component to assign this bug to so be free to reassign as
appropriate.

Comment 1

[Daniel Bratell]

Found the xpidl component in Bugzilla. Reassigning to mccabe.

Comment 2

[Mike McCabe]

Thanks for finding this.

Marking as assigned and changing QA contact.

Comment 3

[Mike McCabe]

Attachment: diff to xpidl_header.c to respect sign and long-ness when generating consts

Comment 4

[Mike McCabe]

Attaching proposed fix.  With m12 and dogfood, it might be a while before I land
it.

bratell -

I'm not totally familiar with just what can go into an enum (signed, unsigned,
long, etc.) or with number type conversions and the printf % formats I used in
the fix.  It seems to work...

Could you take a look and tell me if what I'm doing makes sense?  Am I doing a
lot of extra work to do something that could be handled by automatic conversions
except in the case you noticed?

Thanks -

Mike

Comment 5

[Daniel Bratell]

I'm no guru either but I've applied the patch and the warnings are gone. I'm a
little unsure about the "ld" and "lu" arguments to printf. I _think_ that 'l'
specifies that the given variable is a long variable but I couldn't find the
type of IDL_INTEGER(dcl->const_exp).value so I don't know what's correct. As
both int and long are 32-bit on all modern platforms I'm aware of, it may be a
benign big in the worst case. Apart from that the patch looks good to me.

Comment 6

[Mike McCabe]

*** Bug 22014 has been marked as a duplicate of this bug. ***

Comment 7

[Mike McCabe]

Fix checked in.  Thanks for confirming it :)

Comment 8

[Mike McCabe]

Reopening bug.

I had to revert this fix, because it caused mac, solaris and linux/ppc to
generate '0' for consts.  Needless to say, this lead to trouble elsewhere in the
tree.  I'm looking for clues as to why this triggered a cross-platform
difference... changed file was xpidl_header.c.

Comment 9

[leger]

Clearing FIXED resolution due to reopen.

Comment 10

[Mike McCabe]

Fix checked in.

Comment 11

[leger]

Yes. Old bug.  fixed on all platforms for some time.  Marking Verified.

Comment 12

[Pulsebot]

Pushed by ffxbld-merge:
https://hg.mozilla.org/releases/mozilla-beta/rev/987c72ad1c69
[fenix] fixes https://github.com/mozilla-mobile/fenix/issues/20833: move home recycler bottom margin into standalone item