The default bug view has changed. See this FAQ.

[meta] PRBool(foo & 4) does not produce PR_TRUE, etc.

RESOLVED FIXED in Future

Status

()

Core
General
P4
normal
RESOLVED FIXED
13 years ago
6 years ago

People

(Reporter: brendan, Assigned: (dormant account))

Tracking

(Blocks: 1 bug, {meta})

Trunk
Future
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 8 obsolete attachments)

12.18 KB, patch
Details | Diff | Splinter Review
1.54 KB, patch
Details | Diff | Splinter Review
16.18 KB, application/octet-stream
Details
146.74 KB, patch
Details | Diff | Splinter Review
12.72 KB, text/plain
Details
24.75 KB, patch
Details | Diff | Splinter Review
4.38 KB, text/plain
Details
(Reporter)

Description

13 years ago
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
(Reporter)

Comment 1

13 years ago
Created attachment 163366 [details] [diff] [review]
find | grep with review and hand-editing

Comments welcome.

/be
(Reporter)

Comment 2

13 years ago
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

13 years ago
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

13 years ago
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

13 years ago
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

13 years ago
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

13 years ago
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 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

13 years ago
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.
Product: Browser → Seamonkey
(Reporter)

Updated

12 years ago
Status: NEW → ASSIGNED
Priority: -- → P4
Target Milestone: --- → mozilla1.9alpha1

Updated

12 years ago
Target Milestone: mozilla1.9alpha1 → ---
(Reporter)

Updated

12 years ago
Target Milestone: --- → Future
(Reporter)

Updated

11 years ago
Blocks: 330079
(Reporter)

Comment 10

10 years ago
Taras, care to take this one? Low priority, might be an easy one to fix with an oink tool.

/be
(Assignee)

Comment 11

10 years ago
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?
(Assignee)

Updated

10 years ago
Assignee: brendan → tglek
Status: ASSIGNED → NEW
(Assignee)

Comment 12

10 years ago
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.
(Assignee)

Comment 13

10 years ago
Err. I meant this is a sample list. This is a result of running prcheck on a single file I use for testing.
(Reporter)

Comment 14

10 years ago
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
(Assignee)

Comment 15

10 years ago
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.
Attachment #269240 - Attachment is obsolete: true
(Assignee)

Comment 16

10 years ago
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.
Attachment #269778 - Attachment is obsolete: true
(Assignee)

Comment 17

10 years ago
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?
Attachment #271900 - Attachment is obsolete: true
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.
(Assignee)

Comment 19

10 years ago
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

10 years ago
What exactly is wrong in nsContentList.cpp?
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. 
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.
(Assignee)

Comment 23

10 years ago
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.
> 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

10 years ago
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.
(Assignee)

Comment 26

10 years ago
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

10 years ago
   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());
  }
(Assignee)

Comment 28

10 years ago
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.
Attachment #272242 - Attachment is obsolete: true
(Assignee)

Comment 29

10 years ago
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.
Attachment #272510 - Attachment is obsolete: true

Comment 30

10 years ago
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.
(Reporter)

Comment 31

10 years ago
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

10 years ago
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.
(Assignee)

Comment 33

10 years ago
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.
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
Keywords: meta
(Assignee)

Updated

10 years ago
Depends on: 395822
(Assignee)

Updated

10 years ago
Depends on: 398312
(Assignee)

Comment 34

10 years ago
Created attachment 283250 [details] [diff] [review]
nightly patch

Most recent patch produced by the tool
Attachment #272802 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Depends on: 398433
(Assignee)

Updated

10 years ago
Depends on: 398435
(Assignee)

Updated

10 years ago
Depends on: 398568
(Assignee)

Updated

10 years ago
Depends on: 398571
(Assignee)

Updated

10 years ago
Depends on: 398574
(Assignee)

Updated

10 years ago
Depends on: 398581
(Assignee)

Updated

10 years ago
Depends on: 398587
(Assignee)

Updated

10 years ago
Depends on: 398595
(Assignee)

Updated

10 years ago
Depends on: 398599
(Assignee)

Updated

10 years ago
Depends on: 398623
(Assignee)

Updated

10 years ago
Depends on: 398624
(Assignee)

Comment 35

10 years ago
A lot of people seem to dislike 0 !=.

Should I change it to !!expr?
(Reporter)

Comment 36

10 years ago
See comment 31 and use !! with my blessing.

/be
(Assignee)

Updated

9 years ago
Depends on: 413561
(Assignee)

Updated

9 years ago
Depends on: 417122
(Assignee)

Updated

9 years ago
Depends on: 417125
Taras,
Are you still working on this ?
Component: General → General
Product: Mozilla Application Suite → Core
(Assignee)

Comment 38

9 years ago
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.
(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?
it looks like Taras is using this as a tracking bug. Maybe prefix the summary with a [meta] tag to avoid confusion?
(Assignee)

Updated

9 years ago
Summary: PRBool(foo & 4) does not produce PR_TRUE, etc. → [meta] PRBool(foo & 4) does not produce PR_TRUE, etc.
(Assignee)

Comment 41

9 years ago
Created attachment 337083 [details] [diff] [review]
Latest prbool errors in normal code

Here are the latest errors outside of macros
Attachment #283250 - Attachment is obsolete: true
(Assignee)

Comment 42

9 years ago
Created attachment 337084 [details] [diff] [review]
Prbool stuff involving macros
(Assignee)

Comment 43

9 years ago
Created attachment 337087 [details]
prbool involving macros

Here is a much nicer version of the error list
Attachment #337084 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Depends on: 453889
(Assignee)

Updated

9 years ago
Depends on: 453892
(Assignee)

Updated

9 years ago
Depends on: 454292
(Assignee)

Updated

9 years ago
Depends on: 454419
(Assignee)

Updated

9 years ago
Depends on: 454426
(Assignee)

Updated

9 years ago
Depends on: 454469
(Assignee)

Updated

9 years ago
Depends on: 454502
(Assignee)

Updated

9 years ago
Depends on: 455536
(Assignee)

Updated

9 years ago
No longer depends on: 455536
(Assignee)

Updated

9 years ago
Depends on: 455806
(Assignee)

Updated

8 years ago
No longer depends on: 455806
QA Contact: general → general
Michael, I assume we can close this bug and deps now?

Comment 45

6 years ago
Bug 669808 finished fixing the remaining issues and switching to bool in bug 675553 ensures we shouldn't see this issue again.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.