Last Comment Bug 266048 - [meta] PRBool(foo & 4) does not produce PR_TRUE, etc.
: [meta] PRBool(foo & 4) does not produce PR_TRUE, etc.
Status: RESOLVED FIXED
: meta
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: P4 normal (vote)
: Future
Assigned To: (dormant account)
:
Mentors:
Depends on: 395822 398312 398433 398435 398568 398571 398574 398581 398587 398595 398599 398623 398624 413561 417122 417125 453889 453892 454292 454419 454426 454469 454502
Blocks: 330079
  Show dependency treegraph
 
Reported: 2004-10-25 16:33 PDT by Brendan Eich [:brendan]
Modified: 2011-10-16 18:52 PDT (History)
27 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
find | grep with review and hand-editing (12.18 KB, patch)
2004-10-25 16:34 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
proposed xpcconvert.cpp assertion (1.54 KB, patch)
2004-10-25 16:35 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
Warnings for PRBool (16.18 KB, application/octet-stream)
2004-10-29 21:14 PDT, David Bradley
no flags Details
Fixes errors, and contains PRBool typeded to bool (146.74 KB, patch)
2004-10-29 21:41 PDT, David Bradley
no flags Details | Diff | Splinter Review
Preliminary list of messed up prbools (2.43 KB, text/plain)
2007-06-21 10:53 PDT, (dormant account)
no flags Details
My first patch (3.63 KB, patch)
2007-06-25 17:33 PDT, (dormant account)
no flags Details | Diff | Splinter Review
big sample patch (62.73 KB, patch)
2007-07-11 14:08 PDT, (dormant account)
no flags Details | Diff | Splinter Review
Fixed known bugs in prcheck. processing c files too. (288.76 KB, patch)
2007-07-13 15:21 PDT, (dormant account)
no flags Details | Diff | Splinter Review
Errors that I can not easily fix because they within macros. (12.72 KB, text/plain)
2007-07-13 15:35 PDT, (dormant account)
no flags Details
non-corrupted patch (292.47 KB, patch)
2007-07-16 10:50 PDT, (dormant account)
no flags Details | Diff | Splinter Review
Greatly reduced false posititives (165.87 KB, patch)
2007-07-18 09:52 PDT, (dormant account)
no flags Details | Diff | Splinter Review
nightly patch (55.02 KB, patch)
2007-10-02 14:23 PDT, (dormant account)
no flags Details | Diff | Splinter Review
Latest prbool errors in normal code (24.75 KB, patch)
2008-09-05 11:05 PDT, (dormant account)
no flags Details | Diff | Splinter Review
Prbool stuff involving macros (145.42 KB, patch)
2008-09-05 11:06 PDT, (dormant account)
no flags Details | Diff | Splinter Review
prbool involving macros (4.38 KB, text/plain)
2008-09-05 11:09 PDT, (dormant account)
no flags Details

Description Brendan Eich [:brendan] 2004-10-25 16:33:31 PDT
See bug 265921 comment 5.  Attachment of bad PRBool assignment statements next,
followed by XPConnect patch to assert on non-1 truth values flowing from native
code into JS.

/be
Comment 1 Brendan Eich [:brendan] 2004-10-25 16:34:44 PDT
Created attachment 163366 [details] [diff] [review]
find | grep with review and hand-editing

Comments welcome.

/be
Comment 2 Brendan Eich [:brendan] 2004-10-25 16:35:43 PDT
Created attachment 163367 [details] [diff] [review]
proposed xpcconvert.cpp assertion

Haven't had time to try this yet, it may be painful at first ;-).

/be
Comment 3 David Bradley 2004-10-25 17:52:52 PDT
Probably what would be easier is to declare PRBool as bool, compile with VC++ 7
using warning level 4, or gcc if it has a similar warning, and look for the
specific error, for instance C4305 for VC++ 7. That will catch pretty much
anything that doesn't assign PR_TRUE or PR_FALSE to a PRBool
Comment 4 David Bradley 2004-10-25 22:08:48 PDT
I attempted this tonight but I'm having to deal with various assumptions that
PRBool is PRIntn for instance int nsHashtable's EnumFunc typedef. I'll post a
bug on that tomorrow after I double check to see what it should be. Not sure
what other fun I'll find along the way, but hopefully I'll get the compiler to
generate output that I can grept the warning list.
Comment 5 David Bradley 2004-10-28 18:13:38 PDT
Three days later and I'm still wading through compiler errors. Hopefully soon
I'll have a patch and a list of problems.
Comment 6 David Bradley 2004-10-29 21:14:31 PDT
Created attachment 163927 [details]
Warnings for PRBool

This is the list of warnings (Despite the file name in the zip). I'm going to
work on the error patch, I want to try another thing before I submit it.

We need to move on this quickly, as the line numbers and patch are going to get
stale quickly.

There are over 1600 warnings in the list.
Comment 7 David Bradley 2004-10-29 21:41:53 PDT
Created attachment 163933 [details] [diff] [review]
Fixes errors, and contains PRBool typeded to bool

This is the patch I ended up with. It does two things, it typedef's PRBool to
bool for C++ code, for C it uses an enumeration. I'm not suggesting we check in
the change to the PRBool typedef. I wanted to post it in case someone wanted to
try this as well.

Also just a note about the previous warning list I posted. This is a distilled
list. Not a raw compiler output. I consolidated all the duplicate warnings for
the same file/line.
Comment 8 Christopher Aillon (sabbatical, not receiving bugmail) 2004-10-31 16:01:08 PST
Comment on attachment 163933 [details] [diff] [review]
Fixes errors, and contains PRBool typeded to bool

>Index: nsprpub/pr/include/prtypes.h
>===================================================================
>RCS file: /cvsroot/mozilla/nsprpub/pr/include/prtypes.h,v
>retrieving revision 3.20.4.8
>diff -u -p -r3.20.4.8 prtypes.h
>--- nsprpub/pr/include/prtypes.h	28 Apr 2004 00:33:43 -0000	3.20.4.8
>+++ nsprpub/pr/include/prtypes.h	30 Oct 2004 04:36:33 -0000
>@@ -444,9 +444,17 @@ typedef unsigned long PRUptrdiff;
> **      'if (bool)', 'while (!bool)', '(bool) ? x : y' etc., to test booleans
> **      juast as you would C int-valued conditions. 
> ************************************************************************/
>-typedef PRIntn PRBool;
>-#define PR_TRUE 1
>-#define PR_FALSE 0
>+#ifdef __cplusplus
>+typedef bool PRBool;
>+bool const PR_TRUE = true;
>+bool const PR_FALSE = true;


Oops?  :-)


>+#else
>+typedef enum
>+{
>+    PR_FALSE = 0,
>+    PR_TRUE = 1
>+} PRBool;
>+#endif
Comment 9 David Bradley 2004-10-31 18:33:47 PST
Yes, that was unintential, must of occured after a few tricks I tried to get
things to work in both C and C++.

Just a note, the build doesn't run. I expected as much given the dependance of
XPTCall on types and sizes.
Comment 10 Brendan Eich [:brendan] 2007-06-17 16:43:01 PDT
Taras, care to take this one? Low priority, might be an easy one to fix with an oink tool.

/be
Comment 11 (dormant account) 2007-06-18 10:06:59 PDT
S(In reply to comment #10)
> Taras, care to take this one? Low priority, might be an easy one to fix with an
> oink tool.

Sure. This was impossible to fix before integrating CPP into oink. I should be able to tackle this in the near future. Should I assign the bug to myself?
Comment 12 (dormant account) 2007-06-21 10:53:06 PDT
Created attachment 269240 [details]
Preliminary list of messed up prbools

I think I am mostly done the detection part of my tool. Questions:
1) How should I handle conversion from other bool types (such as PRPackedBool)
I heard that PRPackedBool can end up with negative values, not sure of the details. I'd also need to make a list of bool types that can be assigned directly to PRBool. Alternatively 2) would handle it.

2) Looks like there is a lot of these, should I bother with producing patches automatically? I'm guessing the answer is yes. This would be as simple as wrapping the expression in !!(expr). Shouldn't take more than a few days to implement.
Comment 13 (dormant account) 2007-06-21 11:02:20 PDT
Err. I meant this is a sample list. This is a result of running prcheck on a single file I use for testing.
Comment 14 Brendan Eich [:brendan] 2007-06-21 11:32:33 PDT
Quick reply to comment 12:

1a. PRPackedBool is uint8, so no worries about negatives. You may have been hearing concern about folks using enum-typed single-bit fields, extracting them as PRBool and getting -1 or 0xffffffff on some platforms.

1b. Packed bool can be assigned to bool (JS or PR) type, no problem. Other way wants a cast to silence warnings, and this checker to make sure we aren't trying to store 0x100 (a flag bit masked off from a flags bit-set) into a PRPackedBool, which of course would be a bad bug ("true" turning into false).

2. Automated patches seem to me the only way to fix this big/moving-target code patching problem. The only question is one of style: !!E vs. E != 0. I have so far preferred the latter, and it's less odd-looking to most C and C++ hackers.

Comments from cc: list welcome.

/be
Comment 15 (dormant account) 2007-06-25 17:33:32 PDT
Created attachment 269778 [details] [diff] [review]
My first patch

Good news: I think I'm done with my tool. Attached is the patch output to replace the previous error reports.
It checks return statements in prbool methods, prbool function arguments, variable assignments. It checks JSBool, PRBool and PRPackedBool. Eventually I'll split these checks up to treat some of the types like booleans and some like boolean-compatible ones.
Bad news: MCPP, the C++ processor I'm using can't grok Mozilla yet. It worked fine for the files I was testing but now seems to screw up on many others. I'm looking into it, to see if it can be fixed or if I should just hack up gcc to do what I want. This means that I can't process many files. Tomorrow I'll submit as large of patch as possible given that I can only parse the subset of Mozilla.

Does this patch look good? I'm going to put up the tool in a public repository later this week, would be great if someone could run it through some corner cases.
Comment 16 (dormant account) 2007-07-11 14:08:19 PDT
Created attachment 271900 [details] [diff] [review]
big sample patch

Does anyone want to start committing incremental fixes for this? 
Here is what I can produce currently. There places where code can not be rewritten automatically. This is mostly the case in functions that use PRBool return type and use NS_ENSURE* or similar macros.
There are a few bugs left in my tool mostly to do with pointers. Those should be fixed soon.
Comment 17 (dormant account) 2007-07-13 15:21:51 PDT
Created attachment 272242 [details] [diff] [review]
Fixed known bugs in prcheck. processing c files too.

I'm not sure where to take it from here. There are (2-3) places in the code that expect PRBool contain stuff other than 0/1. Also the patch is gigantic, so it would have to be committed in pieces. 
Indentation is a little messed up, but I fix it up automatically in most cases.

Comments?
Comment 18 Blake Kaplan (:mrbkap) 2007-07-13 15:28:16 PDT
I glanced briefly at the parser/htmlparser/src/CNavDTD.cpp changes and unfortunately, they're incorrect. This is because the code in question is treating PRBool as a tri-state (where -1 is unknown) variable. I'll file a bug and fix it manually, just thought I'd point it out.
Comment 19 (dormant account) 2007-07-13 15:35:59 PDT
Created attachment 272245 [details]
Errors that I can not easily fix because they within macros.

Attached is a list of locations that may return non 0/1 values through macros.

Just to clarify. The patch produced isn't supposed to be a correct fix, but more of a best-guess fix. Main purpose to highlight prbool misuse.
Comment 20 Eli Friedman 2007-07-13 17:42:01 PDT
What exactly is wrong in nsContentList.cpp?
Comment 21 Boris Zbarsky [:bz] 2007-07-13 20:11:42 PDT
Fun.  Cache service defines boolean-valued constants to pass in...

-          PRInt32 semiOffset = style.Find("ch", widthOffset+6);
+          PRInt32 semiOffset = style.Find("ch", 0 != (widthOffset+6));
                             : style.Length() - widthOffset);

is wrong.  That second arg is not a PRBool...

For the rest, if a function returns PRBool, it should be ok to assign it to a PRBool. 
Comment 22 Boris Zbarsky [:bz] 2007-07-13 20:16:56 PDT
Although that string overloading is pretty sucky:

285   NS_HIDDEN_(PRInt32) Find(const char *aStr, PRBool aIgnoreCase = PR_FALSE) const
286   { return Find(aStr, 0, aIgnoreCase); }
287 
288   NS_HIDDEN_(PRInt32) Find(const char *aStr, PRUint32 aOffset, PRBool aIgnoreCase = PR_FALSE) const;

I wonder whether it actually works right...  I guess with the unsigned vs signed thing (PRBool is signed) it does as long as your offset is unsigned.
Comment 23 (dormant account) 2007-07-16 09:05:10 PDT
Eli: 
nothing is wrong with nsContentList.cpp, it's a bug in my tool, thanks.

Boris:
I checked the code and it's actually calling the PRBool overload so this is a bug in Mozilla. In this case the offset is signed so it calls the wrong one.
Comment 24 Boris Zbarsky [:bz] 2007-07-16 09:10:41 PDT
> I checked the code and it's actually calling the PRBool overload 

Fun.  Should be [possible to write a unit test for this, I would think; we should make sure we do that when we make that change.

It sounds like the simplest way to proceed here is to use this as a tracking bug and file separate bugs (per-module perhaps?) for the changes so they can get reviewed...
Comment 25 Eli Friedman 2007-07-16 09:37:45 PDT
I think there's a bug in the patch-making tool in nsInlineFrame.h.  The patch doesn't correspond to the actual text of the file.
Comment 26 (dormant account) 2007-07-16 09:46:01 PDT
In reply to comment #25)
> I think there's a bug in the patch-making tool in nsInlineFrame.h.  The patch
> doesn't correspond to the actual text of the file.
> 

It corresponds to the file here. To make the patch I applied the generated patch to my checkout and then used cvs diff to get a nicer looking patch with window support. Are you sure that your file is recent and on the same branch? I'm using firefox trunk.
Comment 27 Eli Friedman 2007-07-16 10:42:14 PDT
   PRBool IsLeftMost() const {
     // If the frame's bidi visual state is set, return is-leftmost state
     // else return true if it's the first continuation.
-    return (GetStateBits() & NS_INLINE_FRAME_BIDI_VISUAL_STATE_IS_SET)
+    return 0 != ((GetStateBits() & NS_INLINE_FRAME_BIDI_VISUAL_STATE_IS_SET)
-             : (!GetPrevInFlow());
+             : (!GetPrevInFlow()));
   }

vs.

  PRBool IsLeftMost() const {
    // If the frame's bidi visual state is set, return is-leftmost state
    // else return true if it's the first continuation.
    return (GetStateBits() & NS_INLINE_FRAME_BIDI_VISUAL_STATE_IS_SET)
             ? (GetStateBits() & NS_INLINE_FRAME_BIDI_VISUAL_IS_LEFT_MOST)
             : (!GetPrevInFlow());
  }
Comment 28 (dormant account) 2007-07-16 10:50:01 PDT
Created attachment 272510 [details] [diff] [review]
non-corrupted patch

Oops, the patch I was looking at on my hd and the one I uploaded were different. The one I uploaded before was a result of silly bad grep -v \?

Sorry about that.
Comment 29 (dormant account) 2007-07-18 09:52:01 PDT
Created attachment 272802 [details] [diff] [review]
Greatly reduced false posititives

This is a result of a few days of work to get better typedef support in prcheck. Elsa does not provide typedef info in the typesystem, so I had to overlay this on manually. As a result all of the callbacks that were wrongly flagged before are gone now. 
Since chose not to reimplement the C++ typechecker, prcheck will still balk on complicated casts or more complicated typedef indirections. There should be very few of these problem cases.
Would be nice if someone started committing fixes.
Comment 30 Kuno Meyer 2007-07-21 12:12:54 PDT
I just looked on the latest attachment. Wouldn't it make more sense to introduce a PR_NONZERO() macro? Code in the form of ||someBool = 0 != someInt|| looks very distracting to me in terms of operator precedence.

Or a PR_CHECKFLAG(someInt, SOMEFLAG) for the (frequent) cases of ||somebool = 0 != (val & SOMEFLAG)||

Just my two cents.
Comment 31 Brendan Eich [:brendan] 2007-07-21 14:08:38 PDT
I agree it's confusing to see boolvar = 0 != (val & FLAG), but yet another macro is worse. Suggest boolvar = !!(val & FLAG); or boolvar = (val & FLAG) != 0 (put the zero on the right. If precedence is a concern (it isn't for me -- = is loser than all but the comma operator), parenthesize the right hand side.

/be
Comment 32 Kuno Meyer 2007-07-22 11:48:03 PDT
It sounds reasonable not to introduce any more macros. On the other hand, you could rectify this with the argument that for a macro-based type the operators could be macro-based as well.

Putting || != 0 || to the end seems even worse to me, because it tends to get overlooked and forgotten while editing/reorganising.

For me, the most readable of your suggestions is the parenthesizing in the case of assignments to variables. || boolVal = (0 != intVal) ||. For function arguments or return values, this is not needed.
Comment 33 (dormant account) 2007-07-22 17:50:41 PDT
another trick you can do is do static_cast<bool>(). Putting parens around everything tends to get silly in some points, so some special casing is required. 

You can try customizing the tool to make things look nicer. I already have a few special cases. For example I usually rewrite expressions in the form of 0 != (exp) unless exp is a variable, a function call, etc - then I use 0 != exp.

I also tried to make use of prbool casts. Whenever something is cast to a prbool, I rewrite the expression within the cast, instead of outside of a cast.

I don't see a general solution that will be aesthetically appealing in all cases. I also don't think it matters a whole lot, since the patch is only a suggested correction. It's often better to rewrite code with funny precedence and simplify it by hand. 

Most of the patch is fixes for misuses of & operator which are can be applied as is, the rest of too little to bother writing a sophisticated rewriting algorithm for.
Comment 34 (dormant account) 2007-10-02 14:23:53 PDT
Created attachment 283250 [details] [diff] [review]
nightly patch

Most recent patch produced by the tool
Comment 35 (dormant account) 2007-10-04 16:58:40 PDT
A lot of people seem to dislike 0 !=.

Should I change it to !!expr?
Comment 36 Brendan Eich [:brendan] 2007-10-04 17:06:21 PDT
See comment 31 and use !! with my blessing.

/be
Comment 37 Serge Gautherie (:sgautherie) 2008-06-02 14:50:35 PDT
Taras,
Are you still working on this ?
Comment 38 (dormant account) 2008-06-02 15:40:45 PDT
Serge, 
I'm no longer actively working on this. I have a tool that scans code nightly and every once in a while I submit patches with corrections.
Comment 39 Shawn Wilsher :sdwilsh 2008-06-02 16:54:19 PDT
(In reply to comment #38)
> Serge, 
> I'm no longer actively working on this. I have a tool that scans code nightly
> and every once in a while I submit patches with corrections.
Shouldn't this bug then be closed and any new issues that come up be filed as a new bug?
Comment 40 Rob Campbell [:rc] (:robcee) 2008-06-02 16:55:58 PDT
it looks like Taras is using this as a tracking bug. Maybe prefix the summary with a [meta] tag to avoid confusion?
Comment 41 (dormant account) 2008-09-05 11:05:05 PDT
Created attachment 337083 [details] [diff] [review]
Latest prbool errors in normal code

Here are the latest errors outside of macros
Comment 42 (dormant account) 2008-09-05 11:06:05 PDT
Created attachment 337084 [details] [diff] [review]
Prbool stuff involving macros
Comment 43 (dormant account) 2008-09-05 11:09:39 PDT
Created attachment 337087 [details]
prbool involving macros

Here is a much nicer version of the error list
Comment 44 :Ms2ger 2011-10-16 05:00:22 PDT
Michael, I assume we can close this bug and deps now?
Comment 45 Michael Wu [:mwu] 2011-10-16 18:52:37 PDT
Bug 669808 finished fixing the remaining issues and switching to bool in bug 675553 ensures we shouldn't see this issue again.

Note You need to log in before you can comment on or make changes to this bug.