Add memcpy32 #41273

pdietl · 2021-12-16T16:10:59Z

No description provided.

pdietl · 2021-12-16T16:12:16Z

I need @alexanderwachter I need some help testing this.

include/sys/util.h

SebastianBoe · 2021-12-17T09:41:14Z

lib/os/memcpy32.c

Scary gcc attribute. But I assume all supported toolchains are in CI.

Yes this is scary, but there is no other way to get around strict aliasing that I know of :(

Should we add ZRESTRICT to match memcpy() in lib/libc/minimal/include/string.h?

Scary gcc attribute. But I assume all supported toolchains are in CI.

actually they are not.

Only the Zephyr SDK and clang is in CI, but none of the other commercial toolchains.

Other toolchains are gcc or clang based, so if this works with those two, I expect other toolchain should work.
@pdietl can you ensure that this will be tested by CI for minimum Zephyr SDK and clang.

SebastianBoe · 2021-12-17T09:43:46Z

lib/os/memcpy32.c

Strange, I thought restrict was only useful if the same memory address was read multiple times.

But we don't read the same address multiple times in this memcpy.

Did you see any changes to the generated machine code with and without the restrict keyword?

Regular memcpy uses restrict in its parameters, so I did the same here.

I removed it anyway

In Zephyr, you can use ZRESTRICT.

lib/os/CMakeLists.txt

SebastianBoe

Requesting a Kconfig so we don't build this for the vast majority of builds.

pdietl · 2021-12-17T19:46:40Z

@henrikbrixandersen huh that's an interesting error. Plz help :)

pdietl · 2021-12-17T19:51:52Z

Hmm the PDF documentation gen seems to work well

Bu the HTML fails?

alexanderwachter · 2021-12-18T13:49:29Z

@pdietl works on the nucleo_g474re
Edit: sorry. did not switch the branch...

START - test_send_and_forget
E: ***** BUS FAULT *****
E: Imprecise data bus error
E: r0/a1: 0x00000000 r1/a2: 0x15540000 r2/a3: 0x00000008
E: r3/a4: 0x503cb4f3 r12/ip: 0x00000001 r14/lr: 0x08003955
E: xpsr: 0x81000000
E: Faulting instruction address (r15/pc): 0x08001cbc
E: >>> ZEPHYR FATAL ERROR 0: CPU exception on CPU 0
E: Current thread: 0x200001a0 (test_send_and_forget)
E: Halting system

addr2line -a 0x08001cbc -e build/zephyr/zephyr.elf
0x08001cbc
zephyrproject/zephyr/lib/os/memcpy32.c:39 (discriminator 1)

alexanderwachter

Does not work on HW

alexanderwachter · 2021-12-19T10:45:34Z

lib/os/memcpy32.c

Suggested change

while (len >= 0) {

while (len > 0) {

size_t cannot be negative.

tejlmand · 2021-12-21T14:29:05Z

@SebastianBoe Should I make mcan depend on memcpy32 or have mcan select memcpy32 ?

Use select, please.

Are you sure ?
Are we certain that all toolchains support this:
https://github.com/zephyrproject-rtos/zephyr/blob/310e736211c8570469ad42a9b942c720072982c9/lib/os/memcpy32.c#L18

If not, then we could risk ending in a situation where the MEMCPY32 must have a depends on <some toolchain> which again may risk a:

warning: MEMCPY32 (defined at ...) has direct dependencies <SOME_TOOLCHAIN> with value n, but is currently being y-selected by the following symbols:
 - CAN_MCAN (defined at ...), with value y .....

error: Aborting due to Kconfig warnings

when using a select MEMCPY32 here.

As minimum we should ensure Zephyr SDK and clang is supporting __ma_alias, as the toolchains currently supported er either gcc or clang based.

tejlmand

When building tests/drivers/can/canfd/drivers.canfd for nucleo_g474re I get the following errors (warning turned into errors) with twister:

/projects/github/ncs/zephyr/drivers/can/can_mcan.c: In function 'can_mcan_init':
/projects/github/ncs/zephyr/drivers/can/can_mcan.c:389:2: error: converting a packed 'struct can_mcan_msg_sram' pointer (alignment 1) to a 'uint32_t' {aka 'unsigned int'} pointer (alignment 4) may result in an unaligned pointer value [-Werror=address-of-packed-member]
  389 |  for (uint32_t *ptr = (uint32_t *)msg_ram;
      |  ^~~
In file included from /projects/github/ncs/zephyr/drivers/can/can_mcan.c:11:
/projects/github/ncs/zephyr/drivers/can/can_mcan.h:148:8: note: defined here
  148 | struct can_mcan_msg_sram {
      |        ^~~~~~~~~~~~~~~~~
/projects/github/ncs/zephyr/drivers/can/can_mcan.c: In function 'can_mcan_get_message':
/projects/github/ncs/zephyr/drivers/can/can_mcan.c:525:15: error: taking address of packed member of 'struct can_mcan_rx_fifo' may result in an unaligned pointer value [-Werror=address-of-packed-member]
  525 |    for (src = fifo[get_idx].data_32,
      |               ^~~~
/projects/github/ncs/zephyr/drivers/can/can_mcan.c: In function 'can_mcan_send':
/projects/github/ncs/zephyr/drivers/can/can_mcan.c:698:11: error: passing argument 1 of 'memcpy32' discards 'volatile' qualifier from pointer target type [-Werror=discarded-qualifiers]
  698 |  memcpy32(&msg_ram->tx_buffer[put_idx].hdr, &tx_hdr,
      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /projects/github/ncs/zephyr/drivers/can/can_mcan.c:7:
/projects/github/ncs/zephyr/include/sys/util.h:385:32: note: expected 'void * restrict' but argument is of type 'volatile struct can_mcan_tx_buffer_hdr *'
  385 | void *memcpy32(void *ZRESTRICT dest, const void *ZRESTRICT src, size_t n);
      |                ~~~~~~~~~~~~~~~~^~~~
/projects/github/ncs/zephyr/drivers/can/can_mcan.c:702:9: error: taking address of packed member of 'struct can_mcan_tx_buffer' may result in an unaligned pointer value [-Werror=address-of-packed-member]
  702 |   dst = msg_ram->tx_buffer[put_idx].data_32,
      |         ^~~~~~~
cc1: all warnings being treated as errors

tejlmand · 2021-12-21T14:16:25Z

lib/os/memcpy32.c

Scary gcc attribute. But I assume all supported toolchains are in CI.

actually they are not.

Only the Zephyr SDK and clang is in CI, but none of the other commercial toolchains.

Other toolchains are gcc or clang based, so if this works with those two, I expect other toolchain should work.
@pdietl can you ensure that this will be tested by CI for minimum Zephyr SDK and clang.

alexanderwachter · 2021-12-21T15:22:37Z

@tejlmand that should be fixed in #41326

SebastianBoe · 2021-12-21T15:49:36Z

@tejlmand : If a toolchain does not support this then I assume we will get a reasonable error, so I don't see the need for a depends in the Kconfig.

alexanderwachter · 2021-12-21T15:58:38Z

@SebastianBoe @henrikbrixandersen asked for it to speed up compilation time.

Edit:
Was not @henrikbrixandersen.
Think i misread your comment

tejlmand · 2021-12-21T19:22:05Z

@tejlmand : If a toolchain does not support this then I assume we will get a reasonable error, so I don't see the need for a depends in the Kconfig.

except for twister.
There are plans for getting commercial toolchain support in CI, and twister default runs CMake for dts and Kconfig to determine what should be build, and if a board matches the filter, then the test will build for the board.

So just relying on a sane error message build time could impact twister in bad way.
I'm just trying to minimize the risk of having to go through this at later time.

But first thing I would like to know is whether or not the __may_alias is understood by clang, cause if it is, we may not have an immediate problem.

tejlmand · 2021-12-22T07:35:20Z

@tejlmand that should be fixed in #41326

Great, so please rebase this PR, so we can continue.

henrikbrixandersen · 2021-12-22T17:51:39Z

See #40644 for inspiration.

henrikbrixandersen · 2022-01-06T13:10:42Z

@pdietl Do you plan on picking this up again?

pdietl · 2022-01-13T03:58:55Z

Ok what do I need to do to finish this off @henrikbrixandersen

pdietl · 2022-01-13T04:01:11Z

Since memcpy32_volatile was added already to can_mcan.c do we still want or need this?

henrikbrixandersen · 2022-01-13T09:10:06Z

Since memcpy32_volatile was added already to can_mcan.c do we still want or need this?

It would be nice to have a generic memcpy32() available and replace the driver-local implementation with this.

SebastianBoe · 2022-01-13T09:18:01Z

Does memcpy32_volatile not use compiler-specific features? If so, re-writing this to use volatile would resolve the main problems with this PR I believe.

Implement memcpy32, a version of memcpy that only performs 32 bit word-wise copying. Signed-off-by: Pete Dietl <petedietl@gmail.com>

Use memcpy32 for word-wise copying Fixes zephyrproject-rtos#41074 Signed-off-by: Pete Dietl <petedietl@gmail.com>

pdietl · 2022-01-13T22:15:37Z

@SebastianBoe I don't think so?

pdietl · 2022-01-13T22:16:13Z

@henrikbrixandersen Do you want me to change this version of memcpy32 to be volatile?
Regardless I need some help fixing the build failures

tejlmand

Still getting build failures when running twister in tests/drivers/can/canfd/drivers.canfd for nucleo_g474re:

 38%] Building C object zephyr/CMakeFiles/zephyr.dir/lib/os/memcpy32.c.obj
/projects/github/ncs/zephyr/lib/os/memcpy32.c: In function 'memcpy32':
/projects/github/ncs/zephyr/lib/os/memcpy32.c:20:14: error: initialization discards 'volatile' qualifier from pointer target type [-Werror=discarded-qualifiers]
   20 |  char *dst = dst0;
      |              ^~~~
/projects/github/ncs/zephyr/lib/os/memcpy32.c:21:20: error: initialization discards 'volatile' qualifier from pointer target type [-Werror=discarded-qualifiers]
   21 |  const char *src = src0;
      |                    ^~~~
cc1: all warnings being treated as errors
make[2]: *** [zephyr/CMakeFiles/zephyr.dir/build.make:398: zephyr/CMakeFiles/zephyr.dir/lib/os/memcpy32.c.obj] Error 1
make[1]: *** [CMakeFiles/Makefile2:2627: zephyr/CMakeFiles/zephyr.dir/all] Error 2
make: *** [Makefile:91: all] Error 2

pdietl requested review from MaureenHelm, alexanderwachter, andyross, carlescufi, dcpleung, galak and nashif as code owners December 16, 2021 16:11

github-actions bot added area: API Changes to public APIs area: CAN labels Dec 16, 2021

pdietl force-pushed the add_memcpy32 branch from f677167 to d001b8f Compare December 16, 2021 16:22

zephyrbot added area: Base OS Base OS Library (lib/os) area: Build System labels Dec 16, 2021

zephyrbot requested review from SebastianBoe, henrikbrixandersen, karstenkoenig, legoabram, martinjaeger, nixward and tejlmand December 16, 2021 16:57

zephyrbot assigned andyross Dec 16, 2021

pdietl force-pushed the add_memcpy32 branch 2 times, most recently from 8c9f69b to fdf1399 Compare December 16, 2021 17:12

henrikbrixandersen linked an issue Dec 16, 2021 that may be closed by this pull request

can_mcan_send sends corrupted CAN frames with a byte-by-byte memcpy implementation #41074

Closed

SebastianBoe reviewed Dec 17, 2021

View reviewed changes

include/sys/util.h Outdated Show resolved Hide resolved

pdietl force-pushed the add_memcpy32 branch from fdf1399 to 4559ee0 Compare December 17, 2021 09:30

SebastianBoe reviewed Dec 17, 2021

View reviewed changes

lib/os/CMakeLists.txt Outdated Show resolved Hide resolved

SebastianBoe requested changes Dec 17, 2021

View reviewed changes

pdietl force-pushed the add_memcpy32 branch from 6fa4480 to cd47d50 Compare December 17, 2021 15:28

alexanderwachter requested changes Dec 18, 2021

View reviewed changes

alexanderwachter reviewed Dec 19, 2021

View reviewed changes

pdietl force-pushed the add_memcpy32 branch from cd47d50 to b39b7b5 Compare December 19, 2021 16:51

tejlmand requested changes Dec 21, 2021

View reviewed changes

pdietl force-pushed the add_memcpy32 branch 2 times, most recently from 00b3d63 to 9d9f9bd Compare January 13, 2022 21:48

pdietl added 2 commits January 13, 2022 13:50

utils: memcpy32

409a6dd

Implement memcpy32, a version of memcpy that only performs 32 bit word-wise copying. Signed-off-by: Pete Dietl <petedietl@gmail.com>

drivers: mcan

737b827

Use memcpy32 for word-wise copying Fixes zephyrproject-rtos#41074 Signed-off-by: Pete Dietl <petedietl@gmail.com>

pdietl force-pushed the add_memcpy32 branch from 9d9f9bd to 737b827 Compare January 13, 2022 21:51

tejlmand requested changes Feb 1, 2022

View reviewed changes

pdietl closed this Feb 8, 2022

SebastianBoe mentioned this pull request Oct 20, 2022

io32_memcpy #51482

Closed

Uh oh!

Add memcpy32 #41273

Add memcpy32 #41273

Uh oh!

Conversation

pdietl commented Dec 16, 2021

Uh oh!

pdietl commented Dec 16, 2021

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SebastianBoe left a comment

Choose a reason for hiding this comment

Uh oh!

pdietl commented Dec 17, 2021

Uh oh!

pdietl commented Dec 17, 2021

Uh oh!

alexanderwachter commented Dec 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexanderwachter left a comment

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!

tejlmand commented Dec 21, 2021

Uh oh!

tejlmand left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexanderwachter commented Dec 21, 2021

Uh oh!

SebastianBoe commented Dec 21, 2021

Uh oh!

alexanderwachter commented Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tejlmand commented Dec 21, 2021

Uh oh!

tejlmand commented Dec 22, 2021

Uh oh!

henrikbrixandersen commented Dec 22, 2021

Uh oh!

henrikbrixandersen commented Jan 6, 2022

Uh oh!

pdietl commented Jan 13, 2022

Uh oh!

pdietl commented Jan 13, 2022

Uh oh!

henrikbrixandersen commented Jan 13, 2022

Uh oh!

SebastianBoe commented Jan 13, 2022

Uh oh!

pdietl commented Jan 13, 2022

Uh oh!

pdietl commented Jan 13, 2022

Uh oh!

alexanderwachter commented Dec 18, 2021 •

edited

Loading

alexanderwachter commented Dec 21, 2021 •

edited

Loading