Page MenuHomePhabricator

[libcxxabi] Define _LIBCXXABI_GUARD_ABI_ARM on WebAssembly
ClosedPublic

Authored by sbc100 on Thu, Jul 18, 4:30 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

sbc100 created this revision.Thu, Jul 18, 4:30 PM
Herald added a project: Restricted Project. · View Herald Transcript

ARM is the correct monacer for this configuration. The spec we're implementing was written by ARM.

Can you elaborate a bit here about why we're choosing to deviate from the Itanium spec for WebAssembly?
Unless there's good reason why diverging from the Itanium spec is worth it I would rather we not do it.

But my opinion isn't too strong on this. libc++abi has to support this configuration ad infinitum.

My opinion isn't too strong here either. wasm32 doesn't need 8 bytes for this, and there's an existing ABI knob to use less, so it seems to make sense to use it.

I think the main argument is space saving. Wasm cares more about minimizing the binary size and memory usage than most other platforms. I believe that was the original motivation for the arm abi too (they tend to be embedded and care more about doubling the overhead of each guard variable).

The wasm32 ABI as implemented in clang and emscripten currently uses 4-byte guards so we can push back there and try to change the ABI there, or we can change libc++abi to comply the current de-facto ABI.

I originally went with the option of changing clang (and emscripten): https://reviews.llvm.org/D64957

However I think as long as its documented the is no reason not so save a few bytes here in the WebAssembly case.

Can we go ahead with this? Or alternatively land https://reviews.llvm.org/D64957?

sbc100 added a comment.EditedTue, Aug 13, 10:36 AM

What do you think @sunfish, @EricWF? I have a slight preference for landing this change but I want land one or the other.

sbc100 updated this revision to Diff 214878.Tue, Aug 13, 10:40 AM
  • add comment
sunfish accepted this revision.Tue, Aug 13, 12:23 PM

I have a slight preference for landing this change too.

This revision is now accepted and ready to land.Tue, Aug 13, 12:23 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptTue, Aug 13, 6:33 PM