Status

()

Core
MFBT
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: timeless, Assigned: cjones)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
x86
Mac OS X
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

8 years ago
there are a number of warnings which we can quiet by annotating that an assignment is limited to debug statements.

In order to do this, I need a macro which supports:

NS_DEBUG_ASSIGN(lhs) method();
(Reporter)

Comment 1

8 years ago
Created attachment 456726 [details] [diff] [review]
declaration
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #456726 - Flags: review?(benjamin)
(Reporter)

Updated

8 years ago
Blocks: 577904
(Reporter)

Updated

8 years ago
Blocks: 577905
(Reporter)

Updated

8 years ago
Blocks: 577906
(Reporter)

Updated

8 years ago
Blocks: 577907
(Reporter)

Updated

8 years ago
Blocks: 577908
(Reporter)

Updated

8 years ago
Blocks: 577909
(Reporter)

Updated

8 years ago
Blocks: 577910
(Reporter)

Updated

8 years ago
Blocks: 577911
(Reporter)

Updated

8 years ago
Blocks: 577912
(Reporter)

Updated

8 years ago
Blocks: 577913
(Reporter)

Updated

8 years ago
Blocks: 577914
(Reporter)

Updated

8 years ago
Blocks: 577915
(Reporter)

Updated

8 years ago
Blocks: 577916
(Reporter)

Updated

8 years ago
Blocks: 577917
(Reporter)

Updated

8 years ago
Blocks: 577918
(Reporter)

Updated

8 years ago
Blocks: 577919
(Reporter)

Updated

8 years ago
Blocks: 577920
(Reporter)

Updated

8 years ago
Blocks: 577921
Comment on attachment 456726 [details] [diff] [review]
declaration

Excuse the drive-by comment... concerning this:

>+#define NS_DEBUG_ASSIGN(x)             PR_BEGIN_MACRO /* nothing */ PR_END_MACRO;

it seems wrong to let the macro introduce a semicolon here.

Not sure how I feel about the higher-level issue of whether this construct is desirable in the first place. Actually, the more I think about it, the less I like it. It seems to introduce a new opportunity to unintentionally create subtle debug-vs-release differences in our code.
(Reporter)

Comment 3

8 years ago
the semicolon is necessary because of the way PR_BEGIN_MACRO/PR_END_MACRO work.

they result in:

192 #define PR_BEGIN_MACRO  do {
193 #define PR_END_MACRO    } while (0)

so a ; is needed.

you either get:

do {} while (0); bar->Foo();

or:

nsresult rv = bar->Foo();
(Reporter)

Comment 4

8 years ago
I suppose using just "/* nothing */" would be better, as it'd enable:

if (baz)
  NS_DEBUG_ASSIGN(rv) bar->Foo();
IMHO this should be 1 bug not ~20

Comment 6

8 years ago
This is ugly as sin.  NS_DEBUG_ASSIGN(rv, bar->Foo()) looks much nicer to me.
(In reply to comment #6)
> This is ugly as sin.  NS_DEBUG_ASSIGN(rv, bar->Foo()) looks much nicer to me.

The point is that bar->Foo() needs to be evaluated in *both* debug & opt builds, and your suggested syntax in comment 6 would make that less clear, IMHO.  If it's outside of the macro (as it is in timeless's suggested syntax here), it's clear that it always will be evaluated.
(In reply to comment #7)
> (In reply to comment #6)
> > This is ugly as sin.  NS_DEBUG_ASSIGN(rv, bar->Foo()) looks much nicer to me.

Agreed, but....

> The point is that bar->Foo() needs to be evaluated in *both* debug & opt
> builds, and your suggested syntax in comment 6 would make that less clear,
> IMHO.

....also agreed. :(

In either case, I'm still uncomfortable with the way this macro obscures a debug-vs-opt behavioral change that affects following code. Imagine a long function including

    T v = 0;
    ....
    NS_DEBUG_ASSIGN(v) foo->bar();
    NS_WARN_IF_FALSE(v > 0, "expected v to be greater than 0");
    ....

So far so good. But later someone adds a call such as

    foo->baz(v);

further down the function, without noticing that the contents of v at this point is dependent on the type of build. Of course it's possible to create such bugs already with conditionally-compiled fragments, but ISTM this macro would make it easier to inadvertently make such errors.

Would it be preferable to provide a macro such as

    #define NS_UNUSED_IF_RELEASE  __attribute__((unused))

and then append this to the variable declaration concerned?
(Reporter)

Comment 9

8 years ago
Created attachment 456813 [details] [diff] [review]
assign or void

so, I think that it would be relatively hard to make that mistake based on the way the macro is written - it's *LOUD*. Anyone who wants to use a variable needs to decide why they want to use it, which means looking to see who assigned to it which value.

Reviewers could also force people to make their variable declarations NS_DEBUG_DECL (this doesn't exist yet, but we could create it) or #ifdef DEBUG which should make it even harder.

So far all of the changes I've made were of the form where the declaration was guarded by the macro - which means anyone trying to use it in a release build will immediately fall over because the variable isn't defined.

I've been working on mozilla for over a decade mostly paying attention to quirks and strange logic/programming errors, and conditional compilation bugs are incredibly rare - I can't think of the last such instance. And it isn't because we have very little macro magic or because our code is absolutely straightforward.

The primary macro class I can think of which does bite us is this:

#define FOO(x) if (x < 1 || x > 3)
FOO(x++)

I don't think people will be confused by NS_DEBUG_ASSIGN.
Attachment #456726 - Attachment is obsolete: true
Attachment #456813 - Flags: review?(benjamin)
Attachment #456726 - Flags: review?(benjamin)
Yuck. I really don't like this solution, for a couple of reasons:

1) Adding strange macros makes the Mozilla code less approachable, especially at a time when we're trying to make it _more_ approachable.

2) Some of these patches seem like they're just wallpapering over the compiler warning instead of fixing the problem (say, by passing on the error or having the callee return void and assert internally). For example, in bug 577908 you basically have:

    NS_DEBUG_ASSIGN(ok) doSomething();
    NS_ASSERTION(ok, "oops we failed")
    return NS_OK;
How about

#ifdef DEBUG
#define NS_DEBUG_ONLY(x) x
#else
#define NS_DEBUG_ONLY(x)
#endif

Comment 12

8 years ago
Comment on attachment 456813 [details] [diff] [review]
assign or void

Yeah, I don't think this macro is intelligible. I don't mind NS_DEBUG_ONLY if necessary.
Attachment #456813 - Flags: review?(benjamin) → review-
Always give the macro a body, ((void)0) works, to avoid empty statement warnings from some compilers.

/be
(Reporter)

Comment 14

8 years ago
so...

NS_DEBUG_ONLY would look like this:

#define NS_DEBUG_ONLY(lhs) lhs
or
#define NS_DEBUG_ONLY(lhs) (void)

    NS_DEBUG_ONLY(ok =) doSomething();
(Reporter)

Updated

8 years ago
Summary: Provide NS_DEBUG_ASSIGN → Provide NS_DEBUG_ONLY
No, that's not what I wrote, and people already barfed on macros whose bodies are not well-formed expressions or statements (dangling-else immune statements, see {PR,JS}_{BEGIN,END}_MACRO).

What Neil wrote with the fix I suggested looks like this:

#ifdef DEBUG
#define NS_DEBUG_ONLY(x) x
#else
#define NS_DEBUG_ONLY(x) ((void)0)
#endif

/be
(Reporter)

Comment 16

8 years ago
that's not helpful.

the problem case is that the assignment is only used in debug builds. the right hand side needs to be evaluated.
(In reply to comment #16)
> that's not helpful.

See below. At issue is the non-syntactic macro being unsafe and unhelpful on balance.

> the problem case is that the assignment is only used in debug builds. the right
> hand side needs to be evaluated.

I know -- I think (and thought others here agreed) that you should #ifdef DEBUG such declaration and left-hand side followed by =. This is what we do in SpiderMonkey. It's better to avoid non-syntactic magic macros when doing anything like this. Take the #ifdef pain, call the reader's attention to special case.

Example from SpiderMonkey:

#ifdef DEBUG
            PropertyCacheEntry *entry =
#endif
                JS_PROPERTY_CACHE(cx).fill(cx, scopeChain, scopeIndex,
                                           protoIndex, pobj,
                                           (JSScopeProperty *) prop);
            JS_ASSERT(entry);

/be

Comment 18

8 years ago
If we are going to change the code as proposed in attachment 456746 [details] [diff] [review], I'll have to get a C preprocessor brain implant.

I'd prefer the style shown in comment 17.
(Reporter)

Updated

8 years ago
Blocks: 585008
(Reporter)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WONTFIX

Updated

7 years ago
Duplicate of this bug: 589159
OK if NS_DEBUG_DECL doesn't fly, how about a variant of NS_ASSERTION, say NS_CHECK, where the condition is evaluated in non-DEBUG builds? (But nothing special happens if it fails.)
Not reopening yet, but I really liked Jonathon's suggestion:
    #define NS_UNUSED_IF_RELEASE  __attribute__((unused))

But nobody ever mentioned it again. It seems quite clean to me.
There are three problems here, I think
 (1) Silence warnings for locals used only in debug builds
 (2) Don't allocate stack space for debug-only locals
 (3) Don't evaluate rhs's in assignment or initialization of
     debug-only locals if the rhs computation is unnecessary

The last item is the opposite in a sense of the example in comment 17.
AFAIK, the only way to guarantee all three is macro-ization, but I
don't think anyone is too enthused about that.  We can get pretty
close with a template system like

 template<typename T>
 struct DebugOnly {
 #ifdef DEBUG
   /* ... */;
   T v;
 #else
   /* ... */
 #endif
 };

in which
 - both variants of DebugOnly implement default-ctor, copy-ctor, operator=
 - non-DEBUG variant impls are no-ops
 - only the DEBUG impl exposes type-coercion operators etc., to access stored value

This gets us (1), (2), and part of the way to (3).  For an expression like
  DebugOnly<bool> a = ExpensiveSideEffecty();

the compiler won't be able to boil away the call.  Simpler expressions
could be boiled away.  Maybe this isn't a bad thing.

Another sort-of advantage to this scheme is somewhat readable errors
from the compiler if debug-only locals are used in opt builds.  For
example
    bool nd = ExpensiveSideEffecty();
    DebugOnly<bool> db = ExpensiveSideEffecty();
    ASSERT(nd == db);
// ^^^^ always OK
    if (nd == db) { /*...*/ }
// In non-DEBUG builds, gives
//  error: no match for ‘operator==’ in ‘nd == db’
    nd = db;
// In non-DEBUG builds, gives
//  error: cannot convert ‘DebugOnly<bool>’ to ‘bool’ in assignment

Thoughts?
(In reply to comment #22)
> This gets us (1)

Um oops, not this; forgot my -Wall.  We'd need some magic attribute for DebugOnly instances.

Comment 24

7 years ago
Mmmm, I like comment 22.

Just in case, I verified that gcc is actually able to avoid wasting stack space for empty structs and (at least for my simple recursive example), it seems to have no problem.
(In reply to comment #21)
> Not reopening yet, but I really liked Jonathon's suggestion:
>     #define NS_UNUSED_IF_RELEASE  __attribute__((unused))
> 
> But nobody ever mentioned it again. It seems quite clean to me.

Yeah, this seems reasonable to me.

Except I'm worried that people might use it when they really mean for the entire statement rather than just the assignment to be #ifdef DEBUG.

I'm not crazy about using five lines of height:
  #ifdef DEBUG
     nsresult rv =
  #endif
       do_something();
     NS_ASSERTION(NS_SUCCEEDED(rv), "unexpected failure");
where it seems like two lines ought to do, but maybe it's the best solution.

Then again, I think my review- on the layout patch that did that, on grounds of being way too ugly, might have been what prompted this in the first place...
Actually, my suggestion (bug 577914 comment 4) was after this was filed, was never mentioned here, and I review+'d the patch (not review-).

I'd suggested a macro that would give:

  NS_IF_DEBUG(nsIContent* propagatedScrollFrom =) PropagateScrollToViewport();

which I think is a little less magical than timeless's proposals.  But plain old #ifdef DEBUG might still be better.
I'll take a stab at a DebugOnly template after some higher-priority bugs.
Assignee: timeless → jones.chris.g
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(Reporter)

Comment 28

7 years ago
dbaron: re comment 26, i think that's my offer in comment 14 which brendan didn't like in comment 15.
Created attachment 519537 [details] [diff] [review]
part 1: Add mfbt, to contain code shared between SpiderMonkey and Gecko

Tentative rules
 - non-standard files added to mfbt are named LikeThis.h, because that's the style SM and Gecko have converged on
 - non-standard code added to mfbt is in the "mozilla" namespace
 - coding style follows SM, because Gecko coding style is ugly ;)
 - folks will probably know who's best to review code going into mfbt, but a module could be created if necessary

It's not 100% clear how mfbt files will be distributed with SM source releases, but that doesn't seem like a difficult problem.

Please let me know if I've missed folks who should review this.
Attachment #519537 - Flags: superreview?(brendan)
Attachment #519537 - Flags: review?(ted.mielczarek)
Attachment #519537 - Flags: review?(luke)
Created attachment 519538 [details] [diff] [review]
part 2: Add a DebugOnly helper to mfbt, which only contains a value in debug builds
Attachment #519538 - Flags: review?(luke)
Created attachment 519539 [details] [diff] [review]
Example uses of DebugOnly

There are already bugs filed for some of these fixes, but I figured I'd include this for demonstration purposes.
Comment on attachment 519537 [details] [diff] [review]
part 1: Add mfbt, to contain code shared between SpiderMonkey and Gecko

Guess I can't tag more than one sr.
Attachment #519537 - Flags: review?(roc)
Attachment #519537 - Flags: review?(benjamin)
Attachment #519537 - Flags: review?(roc) → review+

Updated

7 years ago
Attachment #519537 - Flags: review?(luke) → review+

Comment 33

7 years ago
Comment on attachment 519538 [details] [diff] [review]
part 2: Add a DebugOnly helper to mfbt, which only contains a value in debug builds

Glad to see this idea landing!
Attachment #519538 - Flags: review?(luke) → review+

Comment 34

7 years ago
Comment on attachment 519539 [details] [diff] [review]
Example uses of DebugOnly

If you want to spread your idea further and get the build-plumbing working for using mozilla/mfbt in js/src,
  :vim /#ifdef DEBUG.*\n.* = *$/ *
points to 12 sites that are begging to be beautified.
Comment on attachment 519538 [details] [diff] [review]
part 2: Add a DebugOnly helper to mfbt, which only contains a value in debug builds

>+ * DebugOnly instances can only be coerced to T in debug builds; in
>+ * release builds, then don't have a value so type coercion is not
>+ * well defined.

I think this wants s/then/they/, right?  (in "then don't have a value")
Definitely!  We've also got what looks to be 20 or so unused variable warnings in gecko that I'm looking to zap once this can land.
(In reply to comment #35)
> I think this wants s/then/they/, right?  (in "then don't have a value")

Oops yep, thanks.

Comment 38

7 years ago
Comment on attachment 519538 [details] [diff] [review]
part 2: Add a DebugOnly helper to mfbt, which only contains a value in debug builds

We should probably also move the static analysis annotations to a shared location: this class should be NS_STACK_CLASS. Note that an empty class as a member will still have a sizeof(1) because that is necessary in order to satisfy C++ requirements about taking its address. In release builds I believe that it will be optimized away as a local variable, but we should probably check that.

Comment 39

7 years ago
Comment on attachment 519537 [details] [diff] [review]
part 1: Add mfbt, to contain code shared between SpiderMonkey and Gecko

How is this going to work in the standalone JS build? It looks to me like this ought to be js/src/mfbt and ought to be built as part of spidermonkey.
Comment on attachment 519537 [details] [diff] [review]
part 1: Add mfbt, to contain code shared between SpiderMonkey and Gecko

># HG changeset patch
># User Chris Jones <jones.chris.g@gmail.com>
># Date 1300229392 18000
># Node ID a6be83c442390b44a9b90a2501b092dfdeaf7163
># Parent  4866be78732f042ba9ffed96fc69a40942d16e26
>Bug 577899, part 1: Add mfbt, to contain code shared between SpiderMonkey and Gecko. r=luke,ted sr=brendan,bsmedberg,roc
>
>diff --git a/config/js/build.mk b/config/js/build.mk
>--- a/config/js/build.mk
>+++ b/config/js/build.mk
>@@ -31,9 +31,9 @@
> # decision by deleting the provisions above and replace them with the notice
> # and other provisions required by the GPL or the LGPL. If you do not delete
> # the provisions above, a recipient may use your version of this file under
> # the terms of any one of the MPL, the GPL or the LGPL.
> #
> # ***** END LICENSE BLOCK *****
> 
> TIERS += js
>-tier_js_dirs = js/src
>+tier_js_dirs = js/src mfbt

This builds mfbt after js/src. Is that really what you want?

>diff --git a/mfbt/Makefile.in b/mfbt/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/mfbt/Makefile.in
>+include $(topsrcdir)/config/config.mk
>+include $(topsrcdir)/config/rules.mk

You don't need to explicitly include config.mk (rules.mk does so).
Attachment #519537 - Flags: review?(ted.mielczarek) → review+
(In reply to comment #38)
> Comment on attachment 519538 [details] [diff] [review]
> part 2: Add a DebugOnly helper to mfbt, which only contains a value in debug
> builds
> 
> We should probably also move the static analysis annotations to a shared
> location: this class should be NS_STACK_CLASS.

Yes, good idea.  Centralizing the annotations would be biting off a fair amount more work, the foundation for which is being laid in bug 642381.  We could block landing DebugOnly on that, but I would be a bit unhappy about that because DebugOnly itself doesn't depend on the deeper problems being worked out in 642381, and not landing DebugOnly will keep going the steady influx of ugly
 #ifdef DEBUG 
   nsresult rv =
 #endif

patches.

As a compromise, how does adding a dummy 
 #define NS_STACK_CLASS

to Util.h plus the "real"
 struct NS_STACK_CLASS DebugOnly
 {

suit?

> In release builds I believe
> that it will be optimized away as a local variable, but we should probably
> check that.

Yes, I checked that.  Judging by comment 24, I believe Luke did too.
(In reply to comment #39)
> Comment on attachment 519537 [details] [diff] [review]
> part 1: Add mfbt, to contain code shared between SpiderMonkey and Gecko
> 
> How is this going to work in the standalone JS build? It looks to me like this
> ought to be js/src/mfbt and ought to be built as part of spidermonkey.

This is being worked out in bug 642381.  Parts 1 and 2 here are dancing around the build problems by having mfbt/ only contain headers.  This is *not* avoiding the distribution problem, but I haven't heard that one is imminent.

Basically, the patches here are deferring all the hard build work to bug 642381, and trying to get in a useful helper (to avoid more ugly DEBUG patches) while that stuff is being sorted out.
(In reply to comment #40)
> Comment on attachment 519537 [details] [diff] [review]
> > TIERS += js
> >-tier_js_dirs = js/src
> >+tier_js_dirs = js/src mfbt
> 
> This builds mfbt after js/src. Is that really what you want?

For this bug it doesn't matter, but I found out in bug 642381 that, no, that is indeed not what I want :).  Thanks.

> You don't need to explicitly include config.mk (rules.mk does so).

Removed.
Comment on attachment 519537 [details] [diff] [review]
part 1: Add mfbt, to contain code shared between SpiderMonkey and Gecko

Not much to see here -- what am I missing.

/be
Attachment #519537 - Flags: superreview?(brendan) → superreview+
Reply/review ping.

Comment 46

7 years ago
Comment on attachment 519537 [details] [diff] [review]
part 1: Add mfbt, to contain code shared between SpiderMonkey and Gecko

From in-person conversation, this should be built from within js/src, so that standalone JS can use it (from a build-ordering perspective). I would vaguely prefer all the files to live in js/mfbt, but I don't have a really strong opinion about it.
Attachment #519537 - Flags: review?(benjamin) → review-
Created attachment 522917 [details] [diff] [review]
part 1: Add mfbt, to contain code shared between SpiderMonkey and Gecko, v2

Moved all build goop into js/src/Makefile.in.  Much cleaner, thanks!
Attachment #519537 - Attachment is obsolete: true
Attachment #522917 - Flags: superreview?(benjamin)

Comment 48

7 years ago
Comment on attachment 522917 [details] [diff] [review]
part 1: Add mfbt, to contain code shared between SpiderMonkey and Gecko, v2

in js/src, $(topsrcdir)/mfbt is js/src/mfbt. Does this patch actually work?
It was working locally because I had an old copy of Util.h in dist/include from the previous version of the patch.  I accidentally pushed this to try and saw that it was busted for clobbers.

The fix was just

+VPATH		+= \
+		$(srcdir)/../../mfbt \
+		$(NULL)

so I didn't think it was important enough to re-post.

Comment 50

7 years ago
Comment on attachment 522917 [details] [diff] [review]
part 1: Add mfbt, to contain code shared between SpiderMonkey and Gecko, v2

r=me the right way, then!
Attachment #522917 - Flags: superreview?(benjamin) → superreview+
Blocks: 647011
http://hg.mozilla.org/mozilla-central/rev/91a8d742c509
http://hg.mozilla.org/mozilla-central/rev/bfef135a83dc
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Keywords: dev-doc-needed
Added https://developer.mozilla.org/index.php?title=En/Namespace/Mozilla/DebugOnly%3CT%3E .  Please let me know if something is unclear.
Comment on attachment 522917 [details] [diff] [review]
part 1: Add mfbt, to contain code shared between SpiderMonkey and Gecko, v2

>diff --git a/mfbt/Util.h b/mfbt/Util.h
>new file mode 100644
>--- /dev/null
>+++ b/mfbt/Util.h
>@@ -0,0 +1,47 @@
>+ * Portions created by the Initial Developer are Copyrigght (C) 2011
>+ * the Initial Developer. All Rights Reserved.

Copy-what?
I cleaned up the doc cjones wrote and added it to the Firefox 5 for developers page. Also added a note about it to:

https://developer.mozilla.org/En/Developer_Guide/Coding_Style#C.2fC.2b.2b_Practices
Keywords: dev-doc-needed → dev-doc-complete
Component: XPCOM → MFBT
QA Contact: xpcom → mfbt
You need to log in before you can comment on or make changes to this bug.