Closed Bug 1004028 Opened 6 years ago Closed 5 years ago

Turn on MOZ_SUPPORT_ASSERT_CONDITION_TYPE_VALIDATION on MSVC versions with sufficiently good decltype support

Categories

(Core :: MFBT, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: bjacob, Assigned: jandem)

References

Details

Attachments

(1 file, 1 obsolete file)

In bug 920292 we're adding checking of the type of arguments passed to MOZ_ASSERT, using decltype. Since MOZ_ASSERT is used on a variety of things, this is a good test for decltype support. At the moment this fails on MSVC 2010 on tryserver, with errors like below. We should eventually revisit and turn this on newer MSVC versions. This should probably wait until newer MSVC versions are covered on TBPL, to avoid breaking the build too frequently on new, relatively little-tested MSVC versions.

https://tbpl.mozilla.org/php/getParsedLog.php?id=38675558&tree=Try#error4


RegExp.cpp

c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\js\src\gc/Heap.h(853) : error C2102: '&' requires l-value

c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\js\src\vm/String.h(318) : error C2352: 'JSString::isLinear' : illegal call of non-static member function

        c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\js\src\vm/String.h(312) : see declaration of 'JSString::isLinear'

c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\js\src\vm/String.h(519) : error C2352: 'JSString::isLinear' : illegal call of non-static member function

        c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\js\src\vm/String.h(312) : see declaration of 'JSString::isLinear'

c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\js\src\vm/String.h(524) : error C2352: 'JSString::isLinear' : illegal call of non-static member function

        c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\js\src\vm/String.h(312) : see declaration of 'JSString::isLinear'

c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\js\src\vm/String.h(570) : error C2352: 'JSString::isFlat' : illegal call of non-static member function

        c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\js\src\vm/String.h(334) : see declaration of 'JSString::isFlat'

c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\js\src\vm/String.h(619) : error C2352: 'JSString::isExtensible' : illegal call of non-static member function

        c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\js\src\vm/String.h(345) : see declaration of 'JSString::isExtensible'

c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\js\src\vm/String.h(707) : error C2352: 'JSString::isExternal' : illegal call of non-static member function

        c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\js\src\vm/String.h(369) : see declaration of 'JSString::isExternal'

c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\js\src\vm/RegExpObject.h(461) : warning C4396: 'js::EmptyShape::ensureInitialCustomShape' : the inline specifier cannot be used when a friend declaration refers to a specialization of a function template

c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\js\src\vm/ErrorObject.h(37) : warning C4396: 'js::EmptyShape::ensureInitialCustomShape' : the inline specifier cannot be used when a friend declaration refers to a specialization of a function template

c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\js\src\vm/StringObject.h(65) : warning C4396: 'js::EmptyShape::ensureInitialCustomShape' : the inline specifier cannot be used when a friend declaration refers to a specialization of a function template

c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/config/rules.mk:1004: recipe for target 'RegExp.obj' failed
Support for MSVC 2012 was removed in bug 1117820, so MSVC 2013 is now the minimum supported version and we should be able to enable this feature for MSVC.
Depends on: 1117820
I actually did a Try push yesterday and it still fails:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=255a7d44925c

js\src\jsgc.h(1205) : error C2102: '&' requires l-value 

That line is: MOZ_ASSERT(JSObject::offsetOfShape() == offsetof(RelocationOverlay, newLocation_));

The build does get pretty far though so I did another Try push with that line commented out:

  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=62e18937afa0

The JS engine uses offsetof() a lot (to access fields from JIT code for instance) so I'm pretty sure there will be similar failures. Also, writing an assertion like this is pretty reasonable and causing Windows-only bustage might do more harm than good.
(In reply to Jan de Mooij [:jandem] from comment #2)
> The JS engine uses offsetof() a lot (to access fields from JIT code for
> instance) so I'm pretty sure there will be similar failures.

Hm the second Try push is green so this was the only problematic assert.

We have two options:

(1) Fix the problematic assert (see comment 2). For instance, we can use a static_assert or use RelocationOverlay::offsetOfNewLocation() instead of offsetof(). Then we can enable MOZ_SUPPORT_ASSERT_CONDITION_TYPE_VALIDATION.

(2) Do nothing, to prevent MSVC-only bustage when people land patches that use MOZ_ASSERT(foo == offsetof(X, y)).

Waldo, WDYT?
Flags: needinfo?(jwalden+bmo)
*Why* does MSVC not like this?  Is it specifically comparing a function call against an offsetof?  What is it particularly that makes this one weird site fail with MSVC?  Potentially it makes the most sense to have a static_assert directly comparing two offsetofs, with RelocationOverlay a friend of JSObject.  But I don't have enough understanding of this to say for sure.

A couple questions/comments beyond the direct goals of this bug, too, since we're in the area and they're somewhat relevant to how this gets adjusted, and to my understanding of what we actually want here.

What's the point of this assertion?  I understand it's trying to make sure overlaps are correct.  But *why* is it important those overlaps be correct?  No matter what happens, this assertion needs a message with it explaining why it's trying to do what it's doing.

Why doesn't magic_ require a similar assertion by it, verifying that magic_ is at the same offset as JSObject::type_?  This would also better explain why RelocationOverlay::Relocated has to have its low bit set (to make it not have the alignment of TypeObject -- a clearer statement of "The low bit is set so this should never equal a normal pointer." which appears by Relocated's definition right now).
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(jdemooij)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> *Why* does MSVC not like this?  Is it specifically comparing a function call
> against an offsetof?  What is it particularly that makes this one weird site
> fail with MSVC?

I don't know. The |'&' requires l-value| part is likely from the offsetof(), which would make sense if that's implemented as a macro (instead of a builtin like GCC/Clang do).

I'll do some Try pushes comparing the result of offsetof() to a constant, another offsetof() etc to narrow it down.

> What's the point of this assertion?

I'm not sure offhand, it's in GC territory. I'll do those Try pushes first to see which patterns are "valid" and what our options are, then I can file a separate JS bug to clean this up.
(In reply to Jan de Mooij [:jandem] from comment #5)
> I'll do those Try pushes first
> to see which patterns are "valid" and what our options are, then I can file
> a separate JS bug to clean this up.

Basically, MSVC (including MSVC 2015) doesn't like decltype(offsetof(X,y)). Here's a test case, reduced from another test case [1]:

    #include <cstddef>

    struct s
    {
      int m1;
    };

    int main()
    {
      decltype(offsetof(s, m1)) n;
      return 0;
    }

[1] https://twitter.com/biikame/status/483095679326121984
Aha!  I see.  So offsetof is something like this, then, probably:

#define offsetof(T, m) \
  (static_cast<std::size_t>(&reinterpret_cast<T*>(sizeof(T))->m) - sizeof(T))

and decltype support bizarrely uses a somewhat different mechanism from sizeof, that requires the provided expression or its components to be an actual thing, where sizeof does not require its argument to be an actual thing.  Okay.

If we only have this one weird case, and we know roughly what triggered it and can hazard guesses as to why, I think we should convert this to a static_assert.  Because offsetof is a compile-time constant, it's generally preferable that people be using it, and not a runtime assertion, for efficiency when debugging (in terms of not having to step over the assertion) and when making changes to associated code (static asserts enforce at compile time).

Most code doesn't use offsetof as much as the JS engine does, and not in assertions.  And when it does, static assertions are usually preferable.  So I think we should convert this one assertion to a static assertion.
OK, patch for this coming up in a bit.
Attached patch Patch (obsolete) — Splinter Review
Converts the assert in jsgc.h to a static_assert and removes MOZ_SUPPORT_ASSERT_CONDITION_TYPE_VALIDATION (all compilers support it now, pretty nice).
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8553336 - Flags: review?(terrence)
Attachment #8553336 - Flags: review?(jwalden+bmo)
Comment on attachment 8553336 [details] [diff] [review]
Patch

Hm, full browser doesn't compile. Leaving r? terrence because fixing this shouldn't affect the JS changes.
Attachment #8553336 - Flags: review?(jwalden+bmo)
Attached patch PatchSplinter Review
Attachment #8553336 - Attachment is obsolete: true
Attachment #8553336 - Flags: review?(terrence)
Attachment #8553347 - Flags: review?(terrence)
Attachment #8553347 - Flags: review?(jwalden+bmo)
Comment on attachment 8553347 [details] [diff] [review]
Patch

Review of attachment 8553347 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsgc.h
@@ +1193,5 @@
>      void forwardTo(Cell *cell) {
>          MOZ_ASSERT(!isForwarded());
> +        static_assert(offsetof(JSObject, shape_) == offsetof(RelocationOverlay, newLocation_),
> +                      "Forwarding pointer and shape should be at same location, "
> +                      "so that cx->zone() works on forwarded objects.");

This should be JSObject::zone()

I better call it a day before I make more silly mistakes.
Comment on attachment 8553347 [details] [diff] [review]
Patch

Review of attachment 8553347 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsgc.h
@@ +1193,5 @@
>      void forwardTo(Cell *cell) {
>          MOZ_ASSERT(!isForwarded());
> +        static_assert(offsetof(JSObject, shape_) == offsetof(RelocationOverlay, newLocation_),
> +                      "Forwarding pointer and shape should be at same location, "
> +                      "so that cx->zone() works on forwarded objects.");

Most compilers, I believe, insert these error messages into a sentence, so uncapitalize and remove the period.

I'd probably write obj->zone(), myself, but either way gets the point across.

::: js/src/jsobj.h
@@ +39,5 @@
>  class ObjectElements;
>  struct StackShape;
>  
> +namespace gc {
> +    class RelocationOverlay;

Fairly sure we don't indent these normally.
Attachment #8553347 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8553347 [details] [diff] [review]
Patch

Review of attachment 8553347 [details] [diff] [review]:
-----------------------------------------------------------------

WFM. I'm sad that JSObject needs to friend details of the GC, but definitely worth it to enable the feature.
Attachment #8553347 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/88849e352ddc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.