Fix for ARMc6 compiler errors #9388

SenRamakri · 2019-01-15T21:05:10Z

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

kjbracey · 2019-01-16T08:00:14Z

features/FEATURE_BLE/source/generic/GenericGap.cpp

@@ -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) {


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.

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?

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

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.

@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)

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.

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.

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 💯 !

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" :)

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

kjbracey · 2019-01-16T09:09:45Z

features/FEATURE_BLE/targets/TARGET_CORDIO/stack/cordio_stack/wsf/common/include/wsf_types.h

@@ -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))


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.

pan- · 2019-01-17T11:31:47Z

features/FEATURE_BLE/source/generic/GenericGap.cpp

@@ -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) {


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) {

pan- · 2019-01-17T11:32:46Z

features/FEATURE_BLE/source/generic/GenericGap.cpp

@@ -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) {


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 · 2019-02-02T00:03:28Z

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

0xc0170 · 2019-02-04T09:37:01Z

Ci started

mbed-ci · 2019-02-04T12:22:54Z

Test run: SUCCESS

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

SenRamakri requested review from kjbracey and pan- January 15, 2019 21:05

cmonr added the needs: review label Jan 16, 2019

kjbracey reviewed Jan 16, 2019

View reviewed changes

pan- mentioned this pull request Jan 16, 2019

BLE: Fix SafeEnum type safety #9393

Merged

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

pan- reviewed Jan 17, 2019

View reviewed changes

SenRamakri and others added 2 commits February 1, 2019 17:46

Fix for ARMc6 compiler errors

ea30ed7

Modified fixes with review comments

2030d03

SenRamakri force-pushed the sen_ArmC6Fixes branch from 250ae3e to 2030d03 Compare February 2, 2019 00:02

kjbracey approved these changes Feb 4, 2019

View reviewed changes

0xc0170 approved these changes Feb 4, 2019

View reviewed changes

0xc0170 added needs: CI release-version: 5.11.4 and removed needs: work labels Feb 4, 2019

pan- approved these changes Feb 4, 2019

View reviewed changes

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

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

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

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

Fix for ARMc6 compiler errors #9388

Fix for ARMc6 compiler errors #9388

Uh oh!

Conversation

SenRamakri commented Jan 15, 2019

Description

Pull request type

Reviewers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjbracey Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjbracey Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SenRamakri commented Feb 2, 2019

Uh oh!

0xc0170 commented Feb 4, 2019

Uh oh!

mbed-ci commented Feb 4, 2019

Uh oh!

Uh oh!

kjbracey Jan 16, 2019 •

edited

Loading

kjbracey Jan 16, 2019 •

edited

Loading