Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for ARMc6 compiler errors #9388

Merged
merged 2 commits into from Feb 4, 2019

Conversation

Projects
None yet
6 participants
@SenRamakri
Copy link
Collaborator

SenRamakri commented Jan 15, 2019

Description

ARMc6 related fixes under BLE feature

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@pan- @kjbracey-arm

@SenRamakri SenRamakri requested review from kjbracey-arm and pan- Jan 15, 2019

@cmonr cmonr added the needs: review label Jan 16, 2019

features/FEATURE_BLE/source/generic/GenericGap.cpp Outdated
@@ -850,7 +850,7 @@ void GenericGap::on_read_phy(
)
{
ble_error_t status = BLE_ERROR_NONE;
if (pal::hci_error_code_t::SUCCESS != hci_status) {
if (pal::hci_error_code_t(pal::hci_error_code_t::SUCCESS) != hci_status) {

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jan 16, 2019

Contributor

It seems like putting in an explicit conversion potentially defeats the point of the type-safety, although I'm not sure as of this C++98 version. Conceptually, at least in newer C++, you'd be activating an explicit conversion, which is a bit like forcing a cast.

I see that just swapping the test to the sane way round clears the problem too:

if (hci_status != pal::hci_error_code_t::SUCCESS) {

Haven't quite understood why yet, but I'll seize on any justification to get people to write their comparisons in the sane order, so I like that.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jan 16, 2019

Contributor

In that order (clang), or either order (other compilers), any integer will be accepted for a comparison:

 if (hci_status != 4) // works on any compiler
 if (4 != hci_status) // works except on clang

They appear to be able to decide that it wants a SafeEnum<hci_error_code_t, uint8_t> to compare with, and locates SafeEnum's implicit conversion constructor to make one. The fact that hci_error_code_t has no implicit conversion constructor has no bearing, as the comparison operators don't act on hci_error_code_t.

It appears that clang on any platform can only perform that deduction if the SafeEnum is the first parameter. This might be a clang bug - afaict first parameter would be important if the operator is a member function, but these aren't, they're friends.

I tried messing a bit to make SafeEnum's constructor explicit, to force the compiler to locate hci_error_code_t's implicit from-type constructor instead, making it type-safe, but it didn't seem to want to find it. (Presumably it doesn't try to convert to a derived class to satisfy a need for a base class).

So, the class isn't working as intended. Presumably you could make a macro form that produced a safe enum with the operators and constructors, rather than using the base class, but maybe it can wait until we update to C++14 and just drop it in favour of enum class, which does everything needed, I think?

This comment has been minimized.

@pan-

pan- Jan 16, 2019

Member

Thanks for the report @kjbracey-arm ; that's interesting. As documented in SafeEnum, the main goal was to avoid scope collision between enum of different types so you don't convert a peer_address_type_t into an own_address_type_t as they have slightly different semantics.

Unfortunately we opened a hole in the type safety with comparison operators operating on the base class and not the derived one. Fortunately it can be fixed by rewriting comparison operators to use Target (the derived class) instead of SafeEnum (live version). Do you think that's enough ? If so I can issue a patch today.

Note that MSVC is also impacted with operand orders. I need to understand why.

Note: As you expressed it, once C++14 is here we can just use enum class

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jan 16, 2019

Contributor

Actually if (4 != hci_status) works on every compiler. clang only complains if the first parameter is an enum type. Issue raised with clang: https://bugs.llvm.org/show_bug.cgi?id=40329

For now, I think just swap the order - that will retain type safety when/if hci_error_code_t becomes enum class : uint8_t.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jan 16, 2019

Contributor

@pan-, ah, the proper-safety fix is simpler than I thought - I was overthinking it. That looks good to me. Go for that.

(But we could still put the comparisons the right way round, on pure taste grounds)

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jan 16, 2019

Contributor

MSVC's error says something about "predefined operator":

'ble::pal::hci_error_code_t::type' does not define this operator or a conversion to a type acceptable to the predefined operator

I think the automatically predefined member operator!=(other) for the enum may be getting in the way - that lookup is only activated when left-hand parameter is of that type. But the non-member candidates should all still be there.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jan 17, 2019

Contributor

clang folks agreed it was a bug, and it has now been fixed upstream - you can now see it working for "clang (trunk)" in compiler explorer.

@SenRamakri - just reverse the comparison for now.

This comment has been minimized.

@0xc0170

0xc0170 Jan 17, 2019

Member

clang folks agreed it was a bug, and it has now been fixed upstream - you can now see it working for "clang (trunk)" in compiler explorer.

@kjbracey-arm 💯 !

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jan 17, 2019

Contributor

Fix visible here for any compiler geeks: llvm-mirror/clang@a7d7e42 (this example is reduced to a test case).

The code modified was clearly marked "FIXME" :)

This comment has been minimized.

@pan-

pan- Jan 17, 2019

Member

Awesome! It already works in clang (trunk) on godbolt.

features/FEATURE_BLE/targets/TARGET_CORDIO/stack/cordio_stack/wsf/common/include/wsf_types.h Outdated
@@ -58,7 +58,7 @@ extern "C" {
/**@{*/
#if ((defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 199901L)) && \
(!defined(__ICC8051__) || (__ICC8051__ == 0))) || \
defined(__CC_ARM) || defined(__IAR_SYSTEMS_ICC__)
defined(__CC_ARM) || defined(__IAR_SYSTEMS_ICC__) || (defined(__ARMCC_VERSION) && (__ARMCC_VERSION >= 6010050))

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jan 16, 2019

Contributor

Do we really need to write things out this long-windedly? The bit being added is a test for "ARMC6 specifically, not ARMC5".

The whole line could just be

defined(__ARMCC_VERSION) || defined(__IAR_SYSTEMS_ICC__)

__ARMCC_VERSION is defined by both ARMC5 and ARMC6, so can just test for that where we handle either.

@cmonr cmonr added needs: work and removed needs: review labels Jan 17, 2019

features/FEATURE_BLE/source/generic/GenericGap.cpp Outdated
@@ -850,7 +850,7 @@ void GenericGap::on_read_phy(
)
{
ble_error_t status = BLE_ERROR_NONE;
if (pal::hci_error_code_t::SUCCESS != hci_status) {
if (pal::hci_error_code_t(pal::hci_error_code_t::SUCCESS) != hci_status) {

This comment has been minimized.

@pan-

pan- Jan 17, 2019

Member
Suggested change
if (pal::hci_error_code_t(pal::hci_error_code_t::SUCCESS) != hci_status) {
if (hci_status != pal::hci_error_code_t::SUCCESS) {
features/FEATURE_BLE/source/generic/GenericGap.cpp Outdated
@@ -867,7 +867,7 @@ void GenericGap::on_phy_update_complete(
)
{
ble_error_t status = BLE_ERROR_NONE;
if (pal::hci_error_code_t::SUCCESS != hci_status) {
if (pal::hci_error_code_t(pal::hci_error_code_t::SUCCESS) != hci_status) {

This comment has been minimized.

@pan-

pan- Jan 17, 2019

Member
Suggested change
if (pal::hci_error_code_t(pal::hci_error_code_t::SUCCESS) != hci_status) {
if (hci_status != pal::hci_error_code_t::SUCCESS) {

@SenRamakri SenRamakri force-pushed the SenRamakri:sen_ArmC6Fixes branch to 2030d03 Feb 2, 2019

@SenRamakri

This comment has been minimized.

Copy link
Collaborator Author

SenRamakri commented Feb 2, 2019

@kjbracey-arm @pan- Please review, the comments are addressed.

@0xc0170

0xc0170 approved these changes Feb 4, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Feb 4, 2019

Ci started

@pan-

pan- approved these changes Feb 4, 2019

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Feb 4, 2019

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 1
Build artifacts

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Feb 4, 2019

@0xc0170 0xc0170 merged commit 026000d into ARMmbed:master Feb 4, 2019

27 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9777 cycles (-746 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@0xc0170 0xc0170 removed the ready for merge label Feb 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.