Opened 9 years ago

Closed 7 years ago

Last modified 7 years ago

#5526 closed enhancement (fixed)

Layout API: Make it FragileBaseClass safe

Reported by: mmadia Owned by: yourpalal
Priority: blocker Milestone: R1/beta1
Component: Kits/Interface Kit Version: R1/Development
Keywords: Cc:
Blocked By: Blocking: #5524
Has a Patch: no Platform: All

Description

As mentioned on this haiku-development mailing list thread, the Layout API needs to be made FragileBaseClass (aka FBC) safe. That is, adding a whole bunch of padding virtuals and padding data to each class as needed.

Change History (16)

comment:1 Changed 9 years ago by mmadia

Summary: Layout Kit: Make it FragileBaseClass safeLayout API: Make it FragileBaseClass safe

comment:2 Changed 8 years ago by yourpalal

Owner: changed from axeld to yourpalal
Status: newassigned

comment:3 Changed 8 years ago by anevilyak

Version: R1/alpha1R1/Development

comment:4 Changed 8 years ago by mmadia

Milestone: R1/beta1
Priority: normalblocker

Setting this as a blocker for entering R1/Beta's.

comment:5 Changed 8 years ago by yourpalal

Now that BObjectList is public, we might as well migrate BLayout and friends to it as well. I mention this here as a reminder, and that it will need to be done before/as part of stabilizing the ABI.

comment:6 Changed 7 years ago by yourpalal

Thinking further, I don't think we have anything to gain by switching to BObjectList in BLayout.

So, that means this ticket boils down to:

  • Adding virtual status_t Perform() methods to all relevant classes. Currently this is inherited from BArchivable, but none of the layout classes implement it.
  • Filling in any other virtual methods that may need to be overridden at a later date with stubs of the form
    foo
    Bar::Baz()
    {
        return Base::Baz();
    }
    
  • Adding virtual method padding (here are my proposed amounts)
    • 5 for BLayoutItem
    • 10 for BLayout (plus the 5 from BLayoutItem)
    • 5 for BTwoDimensionalLayout -> these are inherited by BGroup/BGrid Layout as well, so they probably don't need any of their own
    • BCardLayout/BSplitLayout probably don't need any, I would (mostly) considered these to be leaf classes.
  • Adding uint32 padding: Here I think we could give each class about the same amount of uint32's as they get in virtual methods. For leaf classes, I might add 5 uint32's for good measure.
  • anything else??

One thing I wonder about is whether it is worth it to modify these numbers a bit to get our structures 8 byte-aligned or something. My intuition says it probably won't matter, but maybe I'm wrong :)

comment:7 Changed 7 years ago by yourpalal

I forgot to include:

  • BLayoutContext should get some uint32 padding (maybe to carry extra info about the context), no need for virtuals, I would think.

comment:8 Changed 7 years ago by bonefish

I'd be a bit more generous with virtual method padding. It's rather cheap, since it's only added to the vtable, not to the objects. So 10 slots for every polymorphic class (including BAbstractLayout[Item], B{Group,Grid}Layout,...) is a good idea, I think. The member padding contributes to the object size, so it should be based on how likely additions seem. I would add at least add 2 uint32s to every public class (except basic ones like BSize and BAlignment), maybe up to 8 or 10.

Alignment of object sizes isn't necessary. I also wouldn't move to BObjectList.

Further tasks:

  • Override all virtual methods that one might need to override in the future (including Perform()). They would just call their base class versions, ATM.
  • Add private copy constructor and assignment operator declarations (without implementations) where we don't want to allow copying (in most classes I guess).

comment:9 in reply to:  8 ; Changed 7 years ago by yourpalal

Replying to bonefish:

I'd be a bit more generous with virtual method padding. It's rather cheap, since it's only added to the vtable, not to the objects. So 10 slots for every polymorphic class (including BAbstractLayout[Item], B{Group,Grid}Layout,...) is a good idea, I think.

That sounds reasonable, yeah.

The member padding contributes to the object size, so it should be based on how likely additions seem. I would add at least add 2 uint32s to every public class (except basic ones like BSize and BAlignment), maybe up to 8 or 10.

sure

Further tasks:

  • Override all virtual methods that one might need to override in the future (including Perform()). They would just call their base class versions, ATM.

Hey, I already had that one! :)

  • Add private copy constructor and assignment operator declarations (without implementations) where we don't want to allow copying (in most classes I guess).

Good addition, thanks for your feedback. :)

comment:10 in reply to:  9 ; Changed 7 years ago by bonefish

Replying to yourpalal:

Replying to bonefish:

Further tasks:

  • Override all virtual methods that one might need to override in the future (including Perform()). They would just call their base class versions, ATM.

Hey, I already had that one! :)

Oops, sorry. I read the comment days ago and only skimmed it again before replying. Not sure how I missed that section. I probably did something else concurrently and wasn't as focused as I should have been. I vaguely recall that I had another item as well, but for the life of me can't remember what that might have been.

At any rate, good initiative. It would be great to finally close that chapter.

comment:11 in reply to:  10 ; Changed 7 years ago by yourpalal

Replying to bonefish:

Oops, sorry. I read the comment days ago and only skimmed it again before replying. Not sure how I missed that section. I probably did something else concurrently and wasn't as focused as I should have been.

:) That's alright!

I vaguely recall that I had another item as well, but for the life of me can't remember what that might have been.

We once talked about making BLayoutItem inherit virtually from BArchivable (now's the time), maybe that was it? Checking for public members / inline functions accessing public members (I don't think there are any)? new/delete operators (don't think they're needed)?

At any rate, good initiative. It would be great to finally close that chapter.

Thanks :). Yes, I want to get this done so it doesn't hold up b1, and so that I can work on other stuff!

comment:12 in reply to:  11 Changed 7 years ago by bonefish

Replying to yourpalal:

Replying to bonefish:

I vaguely recall that I had another item as well, but for the life of me can't remember what that might have been.

We once talked about making BLayoutItem inherit virtually from BArchivable (now's the time), maybe that was it?

No. I don't quite recall the details of the discussion, but I don't think that will be necessary.

Checking for public members / inline functions accessing public members (I don't think there are any)?

I can't think of any either, but it certainly doesn't harm to check.

new/delete operators (don't think they're needed)?

I wouldn't think so.

comment:13 Changed 7 years ago by yourpalal

Resolution: fixed
Status: assignedclosed

Fixed in hrev43514 :)

comment:14 Changed 7 years ago by tqh

As pointed out by someone on IRC hrev43514 seems to not be about FBC at all. yourpalal is this really fixed?

comment:15 Changed 7 years ago by tqh

I found it in commits mail, so it seem to be a problem with the tags perhaps.

comment:16 in reply to:  14 Changed 7 years ago by yourpalal

Replying to tqh:

As pointed out by someone on IRC hrev43514 seems to not be about FBC at all. yourpalal is this really fixed?

Yeah, it is fixed, the problem is that the changeset that was tagged as hrev43514 was actually a merge commit that indeed didn't include any changes related to the layout api. Maybe I should have said 'Merge pushed in hrev43514' Furthermore, the 'real' merge was actually a84e14ca84 (one of the parents of hrev43514), after which there was one more change pushed, which was merged in the changeset tagged as hrev43514.

Note: See TracTickets for help on using tickets.