Page MenuHomePhabricator

Disable trivial weak_ptr test on ARM because it is not expected to work.
ClosedPublic

Authored by oontvoo on Jul 20 2020, 2:01 PM.

Details

Summary

weak_ptr has two pointers (more than the 4 bytes limit), so it will not be returned in registers on ARM, even if it is trivial.
The test, therefore, will fail on ARM.

Diff Detail

Event Timeline

oontvoo created this revision.Jul 20 2020, 2:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2020, 2:01 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
This revision was not accepted when it landed; it landed in state Needs Review.Jul 20 2020, 9:18 PM
This revision was automatically updated to reflect the committed changes.
ldionne reopened this revision.Jul 21 2020, 6:13 AM
ldionne added a subscriber: ldionne.

In that case, shouldn't it be an error to try to make weak_ptr trivial on ARM? The current state of things appears to be that one will say "make weak_ptr trivial", and the library will silently fail to honour that request.

oontvoo accepted this revision.Jul 21 2020, 7:27 AM

In that case, shouldn't it be an error to try to make weak_ptr trivial on ARM? The current state of things appears to be that one will say "make weak_ptr trivial", and the library will silently fail to honour that request.

An error by the compiler? IIUC, the attribute is a "request". It can try to honour it but doesn't have to if it can't. (probably similar to how inline and a few other directives are treated)

Or are you suggesting we detect this in the __config and #error (or #warn )? Is it customary for libraries in libcxx to do that?

In that case, shouldn't it be an error to try to make weak_ptr trivial on ARM? The current state of things appears to be that one will say "make weak_ptr trivial", and the library will silently fail to honour that request.

An error by the compiler? IIUC, the attribute is a "request". It can try to honour it but doesn't have to if it can't. (probably similar to how inline and a few other directives are treated)

Or are you suggesting we detect this in the __config and #error (or #warn )? Is it customary for libraries in libcxx to do that?

My point is that someone compiling for ARM will define _LIBCPP_ABI_ENABLE_SHARED_PTR_TRIVIAL_ABI thinking they are getting trivial weak_ptr and shared_ptr passed in registers, but they won't (and silently so). That deception is what I think should be flagged explicitly through an error of some sort. Do you agree?

My point is that someone compiling for ARM will define _LIBCPP_ABI_ENABLE_SHARED_PTR_TRIVIAL_ABI thinking they are getting trivial weak_ptr and shared_ptr passed in registers, but they won't (and silently so). That deception is what I think should be flagged explicitly through an error of some sort. Do you agree?

I can agree that it would seem a bit misleading.

I'm concerned that if we try to get into this business of flagging any potential unsupported case, we'll open a can of worms. That is, the checking here, if we were to be complete, would have to say, if the size of the struct is larger than the limit defined by the target platform, then it's an error. But how do we know what the limit is? There's the theory and there's the practice. For eg, the ARMv7 specs say it reserves 4 registers for the return value, but in practice, it looks like it allows only one (hence the 4 bytes limit). All of these heuristics may turn into an ugly hack (especially if we want to put them in libcxx).

My point is that someone compiling for ARM will define _LIBCPP_ABI_ENABLE_SHARED_PTR_TRIVIAL_ABI thinking they are getting trivial weak_ptr and shared_ptr passed in registers, but they won't (and silently so). That deception is what I think should be flagged explicitly through an error of some sort. Do you agree?

I can agree that it would seem a bit misleading.

I'm concerned that if we try to get into this business of flagging any potential unsupported case, we'll open a can of worms. That is, the checking here, if we were to be complete, would have to say, if the size of the struct is larger than the limit defined by the target platform, then it's an error. But how do we know what the limit is? There's the theory and there's the practice. For eg, the ARMv7 specs say it reserves 4 registers for the return value, but in practice, it looks like it allows only one (hence the 4 bytes limit). All of these heuristics may turn into an ugly hack (especially if we want to put them in libcxx).

Similarly, one could argue the same for unique_ptr for some unknown platform where even a pointer can't be passed by register. Ok, I guess I'm with you now.

Right, the trivial_abi attribute doesn't guarantee that you pass an object in registers, it just gives permission for the implementation to treat the struct as-if it was a trivial struct. On ARM32, since the ABI specifies that trivial structs > 4 bytes are returned indirectly by passing a hidden pointer to a pre-allocated return struct, that's what you get.

oontvoo removed a reviewer: Restricted Project.Jul 24 2020, 8:02 PM

(Closing this ... assuming issue has been resolved)

This revision is now accepted and ready to land.Jul 24 2020, 8:02 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 24 2020, 8:03 PM
This revision now requires review to proceed.Jul 24 2020, 8:03 PM
oontvoo removed a reviewer: Restricted Project.Jul 24 2020, 8:03 PM
This revision is now accepted and ready to land.Jul 24 2020, 8:03 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 24 2020, 8:03 PM
This revision now requires review to proceed.Jul 24 2020, 8:03 PM

Would anyone mind Accepting this so I could clear this revision off my pending queue? Thanks!
(It's already committed so ... )

ldionne accepted this revision.Jul 27 2020, 6:08 AM
This revision is now accepted and ready to land.Jul 27 2020, 6:08 AM
oontvoo closed this revision.Jul 27 2020, 8:04 AM