Page MenuHomePhabricator

[WebAssembly] Fix the order of destructors in the LowerGlobalDtors pass.
ClosedPublic

Authored by sunfish on Nov 25 2019, 10:56 AM.

Details

Summary

Fix the LowerGlobalDtors pass to run destructors in the same order as the regular LLVM destructor lowering -- in reverse order, and not ordered to group destructors with common associated objects.

Diff Detail

Event Timeline

sunfish created this revision.Nov 25 2019, 10:56 AM

the commit description says "not ordered to group destructors with common associated objects" but it looks this patch is doing grouping by associated object. Is that intentional?
Also, maybe there should be a test which has destructors of different priorities associated with the same object?

(also, the langref says that the order within a priority is unspecfied. is something depending on this ordering (I assume so, since you bothered to change it)?

sunfish updated this revision to Diff 251097.Mar 18 2020, 8:33 AM
  • Add a comment.
  • Add a test of two destructors with different priorities and the same associated object.
sunfish added a comment.EditedMar 18 2020, 8:35 AM

The commit description says "not ordered to group destructors with common associated objects" but it looks this patch is doing grouping by associated object. Is that intentional?

It groups adjacent destructors which happen to have the same associated
object, but doesn't order them by the associated object. I've now added a
comment to clarify that.

Also, maybe there should be a test which has destructors of different priorities associated with the same object?

Test added.

(also, the langref says that the order within a priority is unspecfied. is something depending on this ordering (I assume so, since you bothered to change it)?

C++ says we need to run the destructors in reverse order from the
corresponding constructors, so we need to match the order.

dschuff accepted this revision.Mar 18 2020, 10:59 AM

Thanks; please also update the commit message to match your comment update.

This revision is now accepted and ready to land.Mar 18 2020, 10:59 AM
This revision was automatically updated to reflect the committed changes.