This is an archive of the discontinued LLVM Phabricator instance.

Differential D64961

[libcxxabi] Define _LIBCXXABI_GUARD_ABI_ARM on WebAssembly
ClosedPublic

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

Details

Reviewers
EricWF
sunfish
Commits
rCXXA368783: [libcxxabi] Define _LIBCXXABI_GUARD_ABI_ARM on WebAssembly
rGcee41ce223c7: [libcxxabi] Define _LIBCXXABI_GUARD_ABI_ARM on WebAssembly
rL368783: [libcxxabi] Define _LIBCXXABI_GUARD_ABI_ARM on WebAssembly
Summary

This matches ItaniumCXXABI.cpp. Perhaps this should be renamed
to _LIBCXXABI_GUARD_ABI_32BIT.

Fixes PR42680

Diff Detail

Repository
rL LLVM

Event Timeline

sbc100 created this revision.Jul 18 2019, 4:30 PM
Herald added a reviewer: EricWF. · View Herald TranscriptJul 18 2019, 4:30 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: libcxx-commits, christof, sunfish and 4 others. · View Herald Transcript
Harbormaster completed remote builds in B35316: Diff 210702.Jul 18 2019, 4:30 PM
sbc100 added a reviewer: sunfish.Jul 18 2019, 4:31 PM
EricWF added a comment.Jul 18 2019, 7:44 PM

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.

sunfish added a comment.Jul 19 2019, 8:27 AM

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.

sbc100 added a comment.Jul 19 2019, 10:25 AM

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.

sbc100 added a comment.Aug 5 2019, 11:40 AM

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

sbc100 added a comment.EditedAug 13 2019, 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.Aug 13 2019, 10:40 AM
  • add comment
Harbormaster completed remote builds in B36679: Diff 214878.Aug 13 2019, 10:42 AM
sunfish accepted this revision.Aug 13 2019, 12:23 PM

I have a slight preference for landing this change too.

This revision is now accepted and ready to land.Aug 13 2019, 12:23 PM
Closed by commit rL368783: [libcxxabi] Define _LIBCXXABI_GUARD_ABI_ARM on WebAssembly (authored by sbc). · Explain WhyAug 13 2019, 6:33 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2019, 6:33 PM
Herald added a subscriber: llvm-commits. · View Herald Transcript

Revision Contents

PathSize
libcxxabi/
trunk/
include/
3 lines

Diff 215004

libcxxabi/trunk/include/__cxxabi_config.h

Show First 20 Lines • Show All 65 Lines • ▼ Show 20 Lines
#endif #endif
#if __has_attribute(__no_sanitize__) && defined(_LIBCXXABI_COMPILER_CLANG) #if __has_attribute(__no_sanitize__) && defined(_LIBCXXABI_COMPILER_CLANG)
#define _LIBCXXABI_NO_CFI __attribute__((__no_sanitize__("cfi"))) #define _LIBCXXABI_NO_CFI __attribute__((__no_sanitize__("cfi")))
#else #else
#define _LIBCXXABI_NO_CFI #define _LIBCXXABI_NO_CFI
#endif #endif
#if defined(__arm__) // wasm32 follows the arm32 ABI convention of using 32-bit guard.
#if defined(__arm__) || defined(__wasm32__)
# define _LIBCXXABI_GUARD_ABI_ARM # define _LIBCXXABI_GUARD_ABI_ARM
#endif #endif
#endif // ____CXXABI_CONFIG_H #endif // ____CXXABI_CONFIG_H