Closed Bug 1060421 Opened 10 years ago Closed 2 years ago

Change the type of [Int]PointTyped::[x|y] back to [Int]CoordTyped

Categories

(Core :: Graphics: Layers, defect)

Product:
Core
Component:
Graphics: Layers
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
108 Branch
Tracking Flags:
Tracking Status
firefox108 --- fixed

People

(Reporter: botond, Assigned: rzvncj)

References

Details

Attachments

(1 file)

Bug 1060421 - Change the type of [Int]PointTyped::[x|y] back to [Int]CoordTyped. r?botond,dlrobertson
48 bytes, text/x-phabricator-request
Details | Review
This change was originally made in bug 923512, and then backed out in bug 1057642 because it caused a footgun with printf. Once we enable compile-time format string checking for all print-like functions in the tree (bug 1060419), I'd like to change it back.
Bas noticed during the time that the Coord fields were in the tree, that sizeof(Point) had regressed to be larger with Coord fields than it was with float fields. We should investigate this prior to changing the fields back to Coords.
For the record, having this would have caught a units mismatch that was the cause of bug 1230510.
Severity: normal → S3

So to make sure I got this right, you want all [Int]PointTyped classes gone from gfx/2d/Point.h, correct?

The idea is not to remove the [Int]PointTyped classes, but rather to change the type of the x and y fields (stored in the base class BasePoint) from a raw numerical type (T in BasePoint is instantiated with either int32_t, float, or double) to a strongly-typed wrapper. The wrapper type is passed in to BasePoint as the Coord template parameter (IntPointTyped passes IntCoordTyped, and PointTyped passes CoordTyped).

Per comment 1, we should start by checking that this refactoring does not change the size of the structures in question (e.g. we should check sizeof(CSSPoint) and sizeof(CSSIntPoint) and make sure they're the same before and after; we may even want to try and static_assert this somehow).

(In reply to Botond Ballo [:botond] from comment #4)

Per comment 1, we should start by checking that this refactoring does not change the size of the structures in question (e.g. we should check sizeof(CSSPoint) and sizeof(CSSIntPoint) and make sure they're the same before and after; we may even want to try and static_assert this somehow).

I assume that this is because they're being serialized somewhere?

(In reply to Razvan Cojocaru from comment #5)

I assume that this is because they're being serialized somewhere?

No, it's more just that these types are widely used and stored, and the benefits of stronger typing that we would gain from this refactoring are probably not worth a memory usage regression.

(In reply to Botond Ballo [:botond] from comment #4)

Per comment 1, we should start by checking that this refactoring does not change the size of the structures in question (e.g. we should check sizeof(CSSPoint) and sizeof(CSSIntPoint) and make sure they're the same before and after; we may even want to try and static_assert this somehow).

Is the static_assert not already enforced here and here? It looks like any size change will trip it.

(In reply to Razvan Cojocaru from comment #7)

Is the static_assert not already enforced here and here? It looks like any size change will trip it.

Ah, good find! Yeah, those are suitable, so we just need to make sure our changes pass them.

Welp, so much for this patch I suppose:

 0:10.75 In file included from Unified_cpp_accessible_aom0.cpp:2:
 0:10.75 In file included from /mnt/usb/work/debug/mozilla-unified/accessible/aom/AccessibleNode.cpp:11:
 0:10.76 In file included from /mnt/usb/work/debug/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/nsContentUtils.h:26:
 0:10.76 In file included from /mnt/usb/work/debug/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/Units.h:13:
 0:10.76 /mnt/usb/work/debug/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/mozilla/gfx/Point.h:137:5: error: static_assert failed due to requirement 'sizeof(mozilla::gfx::PointTyped<mozilla::LayoutDevicePixel, float>) == sizeof(float) * 2' "Would be unfortunate otherwise!"
 0:10.76     static_assert(sizeof(PointTyped) == sizeof(F) * 2,
 0:10.76     ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 0:10.76 /mnt/usb/work/debug/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/nsIWidget.h:1326:13: note: in instantiation of member function 'mozilla::gfx::PointTyped<mozilla::LayoutDevicePixel>::PointTyped' requested here
 0:10.76             mozilla::LayoutDevicePoint()));
 0:10.76             ^
 0:10.76 In file included from Unified_cpp_accessible_aom0.cpp:2:
 0:10.76 In file included from /mnt/usb/work/debug/mozilla-unified/accessible/aom/AccessibleNode.cpp:11:
 0:10.76 In file included from /mnt/usb/work/debug/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/nsContentUtils.h:26:
 0:10.76 In file included from /mnt/usb/work/debug/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/Units.h:13:
 0:10.76 /mnt/usb/work/debug/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/mozilla/gfx/Point.h:84:5: error: static_assert failed due to requirement 'sizeof(mozilla::gfx::IntPointTyped<mozilla::LayoutDevicePixel>) == sizeof(int) * 2' "Would be unfortunate otherwise!"
 0:10.76     static_assert(sizeof(IntPointTyped) == sizeof(int32_t) * 2,
 0:10.76     ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 0:10.76 /mnt/usb/work/debug/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/mozilla/BasicEvents.h:519:3: note: in instantiation of member function 'mozilla::gfx::IntPointTyped<mozilla::LayoutDevicePixel>::IntPointTyped' requested here
 0:10.76   WidgetEvent() : WidgetEventTime(), mPath(nullptr) {
 0:10.76   ^
 0:10.76 In file included from Unified_cpp_accessible_aom0.cpp:2:
 0:10.76 In file included from /mnt/usb/work/debug/mozilla-unified/accessible/aom/AccessibleNode.cpp:11:
 0:10.76 In file included from /mnt/usb/work/debug/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/nsContentUtils.h:26:
 0:10.76 In file included from /mnt/usb/work/debug/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/Units.h:13:
 0:10.76 /mnt/usb/work/debug/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/mozilla/gfx/Point.h:137:5: error: static_assert failed due to requirement 'sizeof(mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits, float>) == sizeof(float) * 2' "Would be unfortunate otherwise!"
 0:10.77     static_assert(sizeof(PointTyped) == sizeof(F) * 2,
 0:10.77     ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Ok, so a couple of next steps:

  • What are the actual numbers (sizes) in question (on the platform you're building on locally)?
  • Does (for example) IntCoordTyped<CSSPixel> have a different size from int32_t, or is the discrepancy only introduced when putting two values together into BasePoint?

Finally, once we know which structure (BaseCoord or BasePoint) is at issue:

  • Can we get the compiler to lay the structure out without increasing its size, for example using the packed attribute?

Doesn't look like an __attribute__ ((__packed__)) issue. Here's a crass simplification of the Mozilla code:

#include <cstdint>
#include <iostream>

template <class T, class Sub, class Coord = T>
struct BasePoint {
  union {
    struct {
      Coord x, y;
    } s;
    Coord components[2] = {};
  } u;

  constexpr BasePoint() = default;
};

template <class T, class Sub>
struct BaseCoord {
	T value;
};

template <class Units, class Rep>
struct IntCoordTyped
    : public BaseCoord<Rep, IntCoordTyped<Units, Rep>>,
      public Units {
};

template <class Units>
struct IntPointTyped
    : public BasePoint<int32_t, IntPointTyped<Units>, IntCoordTyped<Units, int32_t> >,
      public Units {

  constexpr IntPointTyped() {
    std::cout << "sizeof(IntPointTyped): " << sizeof(IntPointTyped)
        << "\nsizeof(int32_t) * 2: " << sizeof(int32_t) * 2
        << "\nsizeof(u.s.x): " << sizeof(this->u.s.x)
        << "\nsizeof(u.s): " << sizeof(this->u.s)
        << "\nsizeof(u.components): " << sizeof(this->u.components)
        << "\nsizeof(Units): " << sizeof(Units)
        << std::endl;
  }
};

struct UnknownUnits {};

typedef IntPointTyped<UnknownUnits> IntPoint;

int main()
{
    IntPoint ip;
}

This outputs:

sizeof(IntPointTyped): 12
sizeof(int32_t) * 2: 8
sizeof(u.s.x): 4
sizeof(u.s): 8
sizeof(u.components): 8
sizeof(Units): 1

The problem is that IntPointTyped inherits from the empty base Units, as does IntCoordTyped, and the C++ standard apparently wants the compiler to have different addresses for the subobjects. The transformation from the extra 1 byte in size to 4 is just padding.

Here's the relevant part of the ISO C++ standard: "A base class subobject can be of zero size; however, two subobjects that have the same class type and that belong to the same most derived object cannot be allocated at the same address."

If either IntPointTyped or IntCoordTyped stops inheriting Units, the problem goes away (in my test, at least).

Here's an even simpler and hopefully clearer example:

#include <iostream>
#include <cstdint>

using namespace std;

struct Base {
};

struct Derived1 : public Base {
    int32_t member;
};

struct Derived2 : public Base {
    Derived1 member1;
    int32_t member2;
};

struct NotDerived {
    Derived1 member1;
    int32_t member2;
};

int main()
{
    // We expect this to print 4 (sizeof(int32_t), and it does.
    cout << "sizeof(Derived1): " << sizeof(Derived1) << endl;

    // We might expect this to print 8 (2 * sizeof(int32_t),
    // but it doesn't. Instead, it prints 12.
    cout << "sizeof(Derived2): " << sizeof(Derived2) << endl;

    // Finally, we expect this to print 8, and it does.
    cout << "sizeof(NotDerived): " << sizeof(NotDerived) << endl;
}

Thanks for the investigation and reduced example.

(In reply to Razvan Cojocaru from comment #11)

Here's the relevant part of the ISO C++ standard: "A base class subobject can be of zero size; however, two subobjects that have the same class type and that belong to the same most derived object cannot be allocated at the same address."

I wouldn't have thought that the two Base subobjects in Derived2 "belong to the same most derived object" (in my mind, the "most derived object" that they belong to are, respectively, the Derived2 object and the member1 subobject).

However, looking at the normative wording that this note is based on:

[...] Two objects with overlapping lifetimes that are not bit-fields may have the same address if one is nested within the other, or if at least one is a subobject of zero size and they are of different types; otherwise, they have distinct addresses and occupy disjoint bytes of storage.

this does more clearly suggest that the two Base subobjects in this scenario are required to have distinct addresses.

So next, let's evaluate whether we need both PointTyped and CoordTyped to derive from Units.

The reason PointTyped derives from Units is that the unit types define some static member functions like this one, and we like to be able to call these as e.g. CSSPoint::FromAppUnits() rather than CSSPixel::FromAppUnits() (because CSSPoint::FromAppUnits() suggests the CSSPoint return type more clearly).

We do have some methods that return Coord types as well, however, in this case:

  • Writing CSSCoord::FromAppUnits() instead of CSSPixel::FromAppUnits() doesn't make much of a difference for readability
  • Looking through the call sites, they already use the CSSPixel::FromAppUnits() form, with one exception

So, I think it would be reasonable to change CoordTyped and IntCoordTyped to stop inheriting from Units (and change that one affected call site).

One last thing: BaseRect::ClampPoint uses .x and .y of the template type Point, and is being used once with a CSSPoint (for which we need the change to .x.value and .y.value), and once in ExtractRectFromOffset, where Point is a nsPoint, for which we need to continue to use .x and .y directly.

How would you prefer we handle this?

(In reply to Razvan Cojocaru from comment #14)

One last thing: BaseRect::ClampPoint uses .x and .y of the template type Point, and is being used once with a CSSPoint (for which we need the change to .x.value and .y.value), and once in ExtractRectFromOffset, where Point is a nsPoint, for which we need to continue to use .x and .y directly.

How would you prefer we handle this?

Maybe we can change the std::min and std::max calls in the implementation of ClampPoint to calls to new functions CoordMin() and CoordMax(), which have both a generic T overload (that dispatches to std::min/std::max), and a BaseCoord<T, Sub> overload (that unwraps the .value)? We could place this function in BaseCoord.h at namespace scope.

(In reply to Botond Ballo [:botond] from comment #15)

[...] which have both a generic T overload [...] and a BaseCoord<T, Sub> overload [...]

Oh, I guess to make the overload resolution work out we may need overloads for CoordTyped and IntCoordTyped instead of BaseCoord (in which case, we can place them in Coord.h).

(In reply to Botond Ballo [:botond] from comment #16)

(In reply to Botond Ballo [:botond] from comment #15)

[...] which have both a generic T overload [...] and a BaseCoord<T, Sub> overload [...]

Oh, I guess to make the overload resolution work out we may need overloads for CoordTyped and IntCoordTyped instead of BaseCoord (in which case, we can place them in Coord.h).

Right. So you want to keep both uses (I was asking because I thought maybe you'd want to eliminate the nsPoint case altogether).
I've actually done the following, which I thought was the smallest modification that satisfies your requirement:

diff --git a/gfx/2d/BaseRect.h b/gfx/2d/BaseRect.h
--- a/gfx/2d/BaseRect.h
+++ b/gfx/2d/BaseRect.h
@@ -673,8 +673,10 @@ struct BaseRect {
    * edge of the rectangle.
    */
   [[nodiscard]] Point ClampPoint(const Point& aPoint) const {
-    return Point(std::max(x, std::min(XMost(), aPoint.x)),
-                 std::max(y, std::min(YMost(), aPoint.y)));
+    return Point(std::max(decltype(aPoint.x)(x),
+                          std::min(decltype(aPoint.x)(XMost()), aPoint.x)),
+                 std::max(decltype(aPoint.y)(y),
+                          std::min(decltype(aPoint.y)(YMost()), aPoint.y)));
   }
 
   /**

Hope you're OK with it, otherwise I'll switch to specialization / overloading. Just thought we can do without further helper functions adding to the project line count.

[Int]CoordTyped no longer inherits Units because otherwise
instances of [Int]IntPointTyped may get one Base subobject because
it inherits Units, and others because of BasePoint's Coord members,
which end up increasing the [Int]CoordTyped's objects size (since
according to the ISO C++ standard, different Base subobject are
required to have different addresses).

Assignee: nobody → rzvncj
Status: NEW → ASSIGNED

(In reply to Razvan Cojocaru from comment #17)

I've actually done the following, which I thought was the smallest modification that satisfies your requirement:

diff --git a/gfx/2d/BaseRect.h b/gfx/2d/BaseRect.h
--- a/gfx/2d/BaseRect.h
+++ b/gfx/2d/BaseRect.h
@@ -673,8 +673,10 @@ struct BaseRect {
    * edge of the rectangle.
    */
   [[nodiscard]] Point ClampPoint(const Point& aPoint) const {
-    return Point(std::max(x, std::min(XMost(), aPoint.x)),
-                 std::max(y, std::min(YMost(), aPoint.y)));
+    return Point(std::max(decltype(aPoint.x)(x),
+                          std::min(decltype(aPoint.x)(XMost()), aPoint.x)),
+                 std::max(decltype(aPoint.y)(y),
+                          std::min(decltype(aPoint.y)(YMost()), aPoint.y)));
   }
 
   /**

Ah, I see, I misunderstood the problem. I thought the problem was that std::min/std::max did not work with Coord parameters (they actually do, since Coord overloads operator<), but the actual problem is that calls to std::min/std::max with mixed Coord/primitive argument types are ambiguous. Avoiding mixing argument types seems fine to me.

Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1071d7f7a48d Change the type of [Int]PointTyped::[x|y] back to [Int]CoordTyped. r=botond

https://hg.mozilla.org/mozilla-central/rev/1071d7f7a48d

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox108: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 108 Branch
Blocks: 1800530
Blocks: 1812396
Blocks: 1837832
Blocks: 1838100
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: