Bluetooth: Avoid use of array compound literals in API #24829

joerchan · 2020-04-29T18:30:33Z

Attempt to fix C++ error: taking address of temporary array.

According to GCC documentation:

In C++, a compound literal designates a temporary object that only
lives until the end of its full-expression.

Finishing with this recommendation:

As an optimization, G++ sometimes gives array compound literals
longer lifetimes: when the array either appears outside a function
or has a const-qualified type.
But it is probably safest just to avoid the use of array compound
literals in C++ code.

Fix the issue by introducing INIT macros as an alternative, and use
these in the inline functions to avoid the use of array compound
literals in Bluetooth headers.

zephyrbot · 2020-04-29T18:33:10Z

All checks passed.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

a1ien · 2020-04-30T09:00:05Z

Yes, now we can compile c++ file.

a1ien · 2020-04-30T09:02:05Z

But we get warnings

In file included from /home/embedded/Documents/workspace/zephyr_os/zephyr/subsys/bluetooth/services/dis.c:26:
/home/embedded/Documents/workspace/zephyr_os/zephyr/include/bluetooth/gatt.h:554:6: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
  554 |      ((const struct bt_gatt_chrc[]) { {                \
      |      ^
/home/embedded/Documents/workspace/zephyr_os/zephyr/include/bluetooth/gatt.h:450:47: note: in definition of macro 'BT_GATT_SERVICE_DEFINE'
  450 |  const struct bt_gatt_attr attr_##_name[] = { __VA_ARGS__ }; \
      |                                               ^~~~~~~~~~~
/home/embedded/Documents/workspace/zephyr_os/zephyr/include/bluetooth/gatt.h:552:2: note: in expansion of macro 'BT_GATT_ATTRIBUTE'
  552 |  BT_GATT_ATTRIBUTE(BT_UUID_GATT_CHRC, BT_GATT_PERM_READ,             \
      |  ^~~~~~~~~~~~~~~~~
/home/embedded/Documents/workspace/zephyr_os/zephyr/subsys/bluetooth/services/dis.c:108:2: note: in expansion of macro 'BT_GATT_CHARACTERISTIC'
  108 |  BT_GATT_CHARACTERISTIC(BT_UUID_DIS_MODEL_NUMBER,
      |  ^~~~~~~~~~~~~~~~~~~~~~
/home/embedded/Documents/workspace/zephyr_os/zephyr/include/bluetooth/gatt.h:554:6: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
  554 |      ((const struct bt_gatt_chrc[]) { {                \
      |      ^
/home/embedded/Documents/workspace/zephyr_os/zephyr/include/bluetooth/gatt.h:450:47: note: in definition of macro 'BT_GATT_SERVICE_DEFINE'
  450 |  const struct bt_gatt_attr attr_##_name[] = { __VA_ARGS__ }; \
      |                                               ^~~~~~~~~~~
/home/embedded/Documents/workspace/zephyr_os/zephyr/include/bluetooth/gatt.h:552:2: note: in expansion of macro 'BT_GATT_ATTRIBUTE'
  552 |  BT_GATT_ATTRIBUTE(BT_UUID_GATT_CHRC, BT_GATT_PERM_READ,             \
      |  ^~~~~~~~~~~~~~~~~
/home/embedded/Documents/workspace/zephyr_os/zephyr/subsys/bluetooth/services/dis.c:111:2: note: in expansion of macro 'BT_GATT_CHARACTERISTIC'
  111 |  BT_GATT_CHARACTERISTIC(BT_UUID_DIS_MANUFACTURER_NAME,
      |  ^~~~~~~~~~~~~~~~~~~~~~
/home/embedded/Documents/workspace/zephyr_os/zephyr/include/bluetooth/gatt.h:554:6: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
  554 |      ((const struct bt_gatt_chrc[]) { {                \
      |      ^
/home/embedded/Documents/workspace/zephyr_os/zephyr/include/bluetooth/gatt.h:450:47: note: in definition of macro 'BT_GATT_SERVICE_DEFINE'
  450 |  const struct bt_gatt_attr attr_##_name[] = { __VA_ARGS__ }; \
      |                                               ^~~~~~~~~~~
/home/embedded/Documents/workspace/zephyr_os/zephyr/include/bluetooth/gatt.h:552:2: note: in expansion of macro 'BT_GATT_ATTRIBUTE'
  552 |  BT_GATT_ATTRIBUTE(BT_UUID_GATT_CHRC, BT_GATT_PERM_READ,             \
      |  ^~~~~~~~~~~~~~~~~
/home/embedded/Documents/workspace/zephyr_os/zephyr/subsys/bluetooth/services/dis.c:121:2: note: in expansion of macro 'BT_GATT_CHARACTERISTIC'
  121 |  BT_GATT_CHARACTERISTIC(BT_UUID_DIS_SERIAL_NUMBER,
      |  ^~~~~~~~~~~~~~~~~~~~~~
/home/embedded/Documents/workspace/zephyr_os/zephyr/include/bluetooth/gatt.h:554:6: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
  554 |      ((const struct bt_gatt_chrc[]) { {                \
      |      ^
/home/embedded/Documents/workspace/zephyr_os/zephyr/include/bluetooth/gatt.h:450:47: note: in definition of macro 'BT_GATT_SERVICE_DEFINE'
  450 |  const struct bt_gatt_attr attr_##_name[] = { __VA_ARGS__ }; \
      |                                               ^~~~~~~~~~~
/home/embedded/Documents/workspace/zephyr_os/zephyr/include/bluetooth/gatt.h:552:2: note: in expansion of macro 'BT_GATT_ATTRIBUTE'
  552 |  BT_GATT_ATTRIBUTE(BT_UUID_GATT_CHRC, BT_GATT_PERM_READ,             \
      |  ^~~~~~~~~~~~~~~~~
/home/embedded/Documents/workspace/zephyr_os/zephyr/subsys/bluetooth/services/dis.c:127:2: note: in expansion of macro 'BT_GATT_CHARACTERISTIC'
  127 |  BT_GATT_CHARACTERISTIC(BT_UUID_DIS_FIRMWARE_REVISION,
      |  ^~~~~~~~~~~~~~~~~~~~~~
/home/embedded/Documents/workspace/zephyr_os/zephyr/include/bluetooth/gatt.h:554:6: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
  554 |      ((const struct bt_gatt_chrc[]) { {                \
      |      ^
/home/embedded/Documents/workspace/zephyr_os/zephyr/include/bluetooth/gatt.h:450:47: note: in definition of macro 'BT_GATT_SERVICE_DEFINE'
  450 |  const struct bt_gatt_attr attr_##_name[] = { __VA_ARGS__ }; \
      |                                               ^~~~~~~~~~~
/home/embedded/Documents/workspace/zephyr_os/zephyr/include/bluetooth/gatt.h:552:2: note: in expansion of macro 'BT_GATT_ATTRIBUTE'
  552 |  BT_GATT_ATTRIBUTE(BT_UUID_GATT_CHRC, BT_GATT_PERM_READ,             \
      |  ^~~~~~~~~~~~~~~~~
/home/embedded/Documents/workspace/zephyr_os/zephyr/subsys/bluetooth/services/dis.c:132:2: note: in expansion of macro 'BT_GATT_CHARACTERISTIC'
  132 |  BT_GATT_CHARACTERISTIC(BT_UUID_DIS_HARDWARE_REVISION,
      |  ^~~~~~~~~~~~~~~~~~~~~~
/home/embedded/Documents/workspace/zephyr_os/zephyr/include/bluetooth/gatt.h:554:6: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
  554 |      ((const struct bt_gatt_chrc[]) { {                \
      |      ^
/home/embedded/Documents/workspace/zephyr_os/zephyr/include/bluetooth/gatt.h:450:47: note: in definition of macro 'BT_GATT_SERVICE_DEFINE'
  450 |  const struct bt_gatt_attr attr_##_name[] = { __VA_ARGS__ }; \
      |                                               ^~~~~~~~~~~
/home/embedded/Documents/workspace/zephyr_os/zephyr/include/bluetooth/gatt.h:552:2: note: in expansion of macro 'BT_GATT_ATTRIBUTE'
  552 |  BT_GATT_ATTRIBUTE(BT_UUID_GATT_CHRC, BT_GATT_PERM_READ,             \
      |  ^~~~~~~~~~~~~~~~~
/home/embedded/Documents/workspace/zephyr_os/zephyr/subsys/bluetooth/services/dis.c:137:2: note: in expansion of macro 'BT_GATT_CHARACTERISTIC'
  137 |  BT_GATT_CHARACTERISTIC(BT_UUID_DIS_SOFTWARE_REVISION,
      |  ^~~~~~~~~~~~~~~~~~~~~~

joerchan · 2020-04-30T09:04:50Z

@a1ien I know. I will revert the const and do the fix a different way.

joerchan · 2020-04-30T09:16:19Z

@a1ien Could you please check the updated PR?

a1ien · 2020-04-30T10:13:08Z

Now it's ok.

Fix BT_BUF_ACL_SIZE in hci_test_app after declaration was moved to hci_raw.h Signed-off-by: Joakim Andersson <joakim.andersson@nordicsemi.no>

Attempt to fix C++ error: taking address of temporary array. According to GCC documentation: In C++, a compound literal designates a temporary object that only lives until the end of its full-expression. Finishing with this recommendation: As an optimization, G++ sometimes gives array compound literals longer lifetimes: when the array either appears outside a function or has a const-qualified type. But it is probably safest just to avoid the use of array compound literals in C++ code. Fix the issue by introducing INIT macros as an alternative, and use these in the inline functions to avoid the use of array compound literals in Bluetooth headers. Signed-off-by: Joakim Andersson <joakim.andersson@nordicsemi.no>

pabigot · 2020-04-30T18:06:32Z

include/bluetooth/bluetooth.h

-			.peer = (_peer), \
-		 } })
+	((struct bt_le_adv_param[]) { \
+		BT_LE_ADV_PARAM_INIT(_options, _int_min, _int_max, _peer) \


This doesn't work; the cast is still of a temporary. Using a version of samples/bluetooth/peripheral converted to C++:

/mnt/nordic/zp/zephyr/include/bluetooth/bluetooth.h:526:30: error: taking address of temporary array 526 | ((struct bt_le_adv_param[]) { \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~ 527 | BT_LE_ADV_PARAM_INIT(_options, _int_min, _int_max, _peer) \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 528 | }) | ~~ /mnt/nordic/zp/zephyr/include/bluetooth/bluetooth.h:539:29: note: in expansion of macro 'BT_LE_ADV_PARAM' 539 | #define BT_LE_ADV_CONN_NAME BT_LE_ADV_PARAM(BT_LE_ADV_OPT_CONNECTABLE | \ | ^~~~~~~~~~~~~~~ ../src/main.cxx:260:24: note: in expansion of macro 'BT_LE_ADV_CONN_NAME' 260 | err = bt_le_adv_start(BT_LE_ADV_CONN_NAME, ad, ARRAY_SIZE(ad), NULL, 0); | ^~~~~~~~~~~~~~~~~~~

The diagnostic goes away if the cast type is changed to const struct bt_le_adv_param[]

@pabigot Thanks. I didn't add the const-qualifier, because the recommendation stated that was best to avoid these constructs altogether for C++. Which is why I added _INIT macros and removed the use in the static inline functions.
C++ code can then include the header, but it must avoid these specific macros.

If I added const to all the places that used this I get a ripple effect of const qualifier being discarded.

If you change your C++ peripheral like this:

struct bt_le_adv_param param = BT_LE_ADV_PARAM_INIT(BT_LE_ADV_OPT_CONNECTABLE | BT_LE_ADV_OPT_USE_NAME, BT_GAP_ADV_FAST_INT_MIN_2, BT_GAP_ADV_FAST_INT_MAX_2, NULL); err = bt_le_adv_start(&param, ad, ARRAY_SIZE(ad), NULL, 0);

You should be good.

OK, I accept that's a workaround so I'll remove my NAK. It'd be nice to have uses of those macros in C++ code produce a diagnostic, but that's not in scope here.

The Zephyr Bluetooth examples are awash in uses of the original (non-init) macro as an argument to functions. A goal of the original solution was to keep the recommended calling code the same for both C and C++ code. Clearly that's not feasible.

We should probably add or convert one of the bluetooth samples to C++ to show how it's supposed to be done.

FWIW include/bluetooth/gatt.h at BT_GATT_CHARACTERISTIC also has an explicit cast of a temporary compound literal being cast to an array. That may not be as easy to fix, but also need not be part of this PR.

giving up on goal of using same idiom for C and C++ code

pabigot · 2020-05-02T16:13:41Z

If you change your C++ peripheral like this:

struct bt_le_adv_param param = BT_LE_ADV_PARAM_INIT(BT_LE_ADV_OPT_CONNECTABLE |
						    BT_LE_ADV_OPT_USE_NAME,
						    BT_GAP_ADV_FAST_INT_MIN_2,
						    BT_GAP_ADV_FAST_INT_MAX_2,
						    NULL);
err = bt_le_adv_start(&param, ad, ARRAY_SIZE(ad), NULL, 0);

You should be good.

It can also be changed like this:

const auto& name = BT_LE_ADV_CONN_NAME;
err = bt_le_adv_start(name, ad, ARRAY_SIZE(ad), NULL, 0);

which is a far simpler workaround that allows continued use of the helper macros.

This should be legal because C++20 section 6.7.7 paragraph 6.1 confirms that binding a reference to a temporary xvalue materialized by decaying (7.3.2) the array prvalue produced by (type[]){{...}} delays the destruction of the temporary for the lifetime of the reference.

Or, if there's concern that all this does is delay the destruction of the temporary decayed pointer rather than the temporary it points to:

	auto name = BT_LE_ADV_CONN_NAME[0];
	err = bt_le_adv_start(&name, ad, ARRAY_SIZE(ad), NULL, 0);

which is still better.

joerchan mentioned this pull request Apr 29, 2020

Bluetooth: host: Add extended advertising API #22013

Merged

zephyrbot added area: Bluetooth area: API Changes to public APIs area: Tests Issues related to a particular existing or missing test labels Apr 29, 2020

joerchan force-pushed the bt-cpp-error-compound-array-literal branch 2 times, most recently from 9f24a91 to 9aee2e2 Compare April 30, 2020 09:13

joerchan changed the title ~~Bt cpp error compound array literal~~ Bluetooth: Avoid use of array compound literals in API Apr 30, 2020

joerchan force-pushed the bt-cpp-error-compound-array-literal branch from 9aee2e2 to 1dbcf35 Compare April 30, 2020 11:10

joerchan marked this pull request as ready for review April 30, 2020 11:11

joerchan requested review from aescolar, jhedberg, Vudentz and wopu-ot as code owners April 30, 2020 11:11

joerchan added 2 commits April 30, 2020 17:26

Bluetooth: test: Fix redefinition of BT_BUF_ACL_SIZE

f55cf9a

Fix BT_BUF_ACL_SIZE in hci_test_app after declaration was moved to hci_raw.h Signed-off-by: Joakim Andersson <joakim.andersson@nordicsemi.no>

joerchan force-pushed the bt-cpp-error-compound-array-literal branch from 1dbcf35 to ecfd6d0 Compare April 30, 2020 15:28

pabigot previously requested changes Apr 30, 2020

View reviewed changes

jhedberg approved these changes May 2, 2020

View reviewed changes

jhedberg merged commit 78a6cdb into zephyrproject-rtos:master May 2, 2020

joerchan deleted the bt-cpp-error-compound-array-literal branch May 3, 2020 14:42

joerchan mentioned this pull request May 4, 2020

Bluetooth: host: Fix BT_CONN_LE_OPT_NONE renamed issue #24941

Merged

pabigot mentioned this pull request Jan 16, 2021

sensors: BME280: obtain device pointers directly #30536

Merged

pabigot mentioned this pull request Jan 31, 2021

address-of-temporary idiom not allowed in C++ #18551

Closed

jhedberg mentioned this pull request Nov 16, 2021

Bluetooth: removing temporary array initialization #40401

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bluetooth: Avoid use of array compound literals in API #24829

Bluetooth: Avoid use of array compound literals in API #24829

joerchan commented Apr 29, 2020 •

edited

Loading

zephyrbot commented Apr 29, 2020 •

edited

Loading

a1ien commented Apr 30, 2020

a1ien commented Apr 30, 2020

joerchan commented Apr 30, 2020

joerchan commented Apr 30, 2020

a1ien commented Apr 30, 2020

pabigot Apr 30, 2020

joerchan Apr 30, 2020

pabigot Apr 30, 2020

pabigot commented May 2, 2020 •

edited

Loading

Bluetooth: Avoid use of array compound literals in API #24829

Bluetooth: Avoid use of array compound literals in API #24829

Conversation

joerchan commented Apr 29, 2020 • edited Loading

zephyrbot commented Apr 29, 2020 • edited Loading

a1ien commented Apr 30, 2020

a1ien commented Apr 30, 2020

joerchan commented Apr 30, 2020

joerchan commented Apr 30, 2020

a1ien commented Apr 30, 2020

pabigot Apr 30, 2020

Choose a reason for hiding this comment

joerchan Apr 30, 2020

Choose a reason for hiding this comment

pabigot Apr 30, 2020

Choose a reason for hiding this comment

pabigot commented May 2, 2020 • edited Loading

joerchan commented Apr 29, 2020 •

edited

Loading

zephyrbot commented Apr 29, 2020 •

edited

Loading

pabigot commented May 2, 2020 •

edited

Loading