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

TLV reader: Fix float support and remove union UB #8531

Merged
merged 2 commits into from
Jul 30, 2021

Conversation

anqid-g
Copy link
Contributor

@anqid-g anqid-g commented Jul 21, 2021

  1. For some reason the Get(float&) signature was declared in the .h file
    but never defined. Define it as well, extracting a helper function
    for both Get(float&) and Get(double&).
  2. It is undefined behaviour to use unions for type-punning in C++. Use
    raw memcpy calls instead, which is well-defined (std::bit_cast is not
    available). Compilers will generally be able to optimize this away
    unless a freestanding implementation (with possibly non-standard
    memcpy) is used.

Problem

Various improvements to TLVReader floating point code.

Change overview

  1. Implement TLVReader::Get(float&)
  2. Use memcpy instead of type-punning for bit conversions from integer to floating-point

Testing

  • Updated TLVReader tests by copy-pasting the existing test cases for double

@boring-cyborg boring-cyborg bot added the lib label Jul 21, 2021
@CLAassistant
Copy link

CLAassistant commented Jul 21, 2021

CLA assistant check
All committers have signed the CLA.

@anqid-g anqid-g force-pushed the tlv-reader-floats branch 2 times, most recently from b90419b to 55feaff Compare July 21, 2021 17:17
@anqid-g anqid-g force-pushed the tlv-reader-floats branch 2 times, most recently from dc5fa54 to 7b07d8f Compare July 21, 2021 19:17
@github-actions
Copy link

Size increase report for "esp32-example-build" from af1cc26

File Section File VM
chip-lock-app.elf .flash.text 68 68
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-ipv6only-app.elf and ./pull_artifact/chip-ipv6only-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_loc,0,377
.debug_info,0,199
.debug_ranges,0,64
.debug_str,0,59
.debug_line,0,58
.debug_abbrev,0,48
.debug_frame,0,40
.debug_aranges,0,8
.riscv.attributes,0,-1

Comparing ./master_artifact/chip-temperature-measurement-app.elf and ./pull_artifact/chip-temperature-measurement-app.elf:

sections,vmsize,filesize
.debug_loc,0,268
.debug_info,0,212
.debug_line,0,92
.debug_ranges,0,64
.debug_str,0,60
.debug_abbrev,0,48
.debug_frame,0,24
.debug_aranges,0,8

Comparing ./master_artifact/chip-lock-app.elf and ./pull_artifact/chip-lock-app.elf:

sections,vmsize,filesize
.debug_info,0,209
.debug_loc,0,175
.debug_line,0,84
.flash.text,68,68
.debug_ranges,0,56
.debug_str,0,56
.debug_abbrev,0,48
.debug_frame,0,24
.debug_aranges,0,8
[Unmapped],0,-68

Comparing ./master_artifact/chip-persistent-storage.elf and ./pull_artifact/chip-persistent-storage.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize


src/lib/core/CHIPTLVReader.cpp Outdated Show resolved Hide resolved
src/lib/core/CHIPTLVReader.cpp Outdated Show resolved Hide resolved
@anqid-g anqid-g changed the title TLV reader: Fix floating support TLV reader: Fix float support Jul 22, 2021
@anqid-g anqid-g changed the title TLV reader: Fix float support TLV reader: Fix float support and remove union UB Jul 22, 2021
Copy link
Contributor

@bzbarsky-apple bzbarsky-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful, thank you!

@bzbarsky-apple
Copy link
Contributor

/rebase

anqid-g added 2 commits July 29, 2021 11:37
1. For some reason the Get(float&) signature was declared in the .h file
   but was never defined. Define it as well. Disallow reading a 64-bit
   floating point number to float, to avoid silent truncation / loss of
   precision.
2. It is undefined behaviour to use unions for type-punning in C++. Use
   raw memcpy calls instead, which is defined by the C++ standard
   (std::bit_cast is not available).

   Compilers will generally be able to optimize this memcpy away, unless
   a freestanding implementation (with possibly non-standard memcpy) is
   used.
It is undefined behaviour to use unions for type-punning in C++. Use
raw memcpy calls instead, as a substitute for the unavailable
std::bit_cast.
@anqid-g anqid-g force-pushed the tlv-reader-floats branch from d37a104 to f31add3 Compare July 29, 2021 18:39
@bzbarsky-apple bzbarsky-apple merged commit e92ef34 into project-chip:master Jul 30, 2021
anqid-g added a commit to anqid-g/openweave-core that referenced this pull request Jul 30, 2021
1. For some reason the Get(float&) signature was declared in the .h file
   but was never defined. Define it as well. Disallow reading a 64-bit
   floating point number to float, to avoid silent truncation / loss of
   precision.
2. It is undefined behaviour to use unions for type-punning in C++. Use
   raw memcpy calls instead, which is defined by the C++ standard
   (std::bit_cast is not available).

   Compilers will generally be able to optimize this memcpy away, unless
   a freestanding implementation (with possibly non-standard memcpy) is
   used.

This is a backport from an analogous commit to CHIP:
project-chip/connectedhomeip#8531
rcasallas-silabs pushed a commit to rcasallas-silabs/connectedhomeip that referenced this pull request Aug 2, 2021
* TLV reader: Improve floating-point support

1. For some reason the Get(float&) signature was declared in the .h file
   but was never defined. Define it as well. Disallow reading a 64-bit
   floating point number to float, to avoid silent truncation / loss of
   precision.
2. It is undefined behaviour to use unions for type-punning in C++. Use
   raw memcpy calls instead, which is defined by the C++ standard
   (std::bit_cast is not available).

   Compilers will generally be able to optimize this memcpy away, unless
   a freestanding implementation (with possibly non-standard memcpy) is
   used.

* TLV writer: Remove union UB

It is undefined behaviour to use unions for type-punning in C++. Use
raw memcpy calls instead, as a substitute for the unavailable
std::bit_cast.
anqid-g added a commit to anqid-g/openweave-core that referenced this pull request Aug 3, 2021
1. For some reason the Get(float&) signature was declared in the .h file
   but was never defined. Define it as well. Disallow reading a 64-bit
   floating point number to float, to avoid silent truncation / loss of
   precision.
2. It is undefined behaviour to use unions for type-punning in C++. Use
   raw memcpy calls instead, which is defined by the C++ standard
   (std::bit_cast is not available).

   Compilers will generally be able to optimize this memcpy away, unless
   a freestanding implementation (with possibly non-standard memcpy) is
   used.

This is a backport from an analogous commit to CHIP:
project-chip/connectedhomeip#8531

GitHub commit: 7de322b

Change-Id: I53828d172066d433114a2584636af54fd7dbaacd
anqid-g added a commit to anqid-g/openweave-core that referenced this pull request Aug 3, 2021
1. For some reason the Get(float&) signature was declared in the .h file
   but was never defined. Define it as well. Disallow reading a 64-bit
   floating point number to float, to avoid silent truncation / loss of
   precision.
2. It is undefined behaviour to use unions for type-punning in C++. Use
   raw memcpy calls instead, which is defined by the C++ standard
   (std::bit_cast is not available).

   Compilers will generally be able to optimize this memcpy away, unless
   a freestanding implementation (with possibly non-standard memcpy) is
   used.

This is a backport from an analogous commit to CHIP:
project-chip/connectedhomeip#8531
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
* TLV reader: Improve floating-point support

1. For some reason the Get(float&) signature was declared in the .h file
   but was never defined. Define it as well. Disallow reading a 64-bit
   floating point number to float, to avoid silent truncation / loss of
   precision.
2. It is undefined behaviour to use unions for type-punning in C++. Use
   raw memcpy calls instead, which is defined by the C++ standard
   (std::bit_cast is not available).

   Compilers will generally be able to optimize this memcpy away, unless
   a freestanding implementation (with possibly non-standard memcpy) is
   used.

* TLV writer: Remove union UB

It is undefined behaviour to use unions for type-punning in C++. Use
raw memcpy calls instead, as a substitute for the unavailable
std::bit_cast.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants