raspberrypi / utils

A collection of scripts and simple applications
BSD 3-Clause "New" or "Revised" License
136 stars 41 forks source link

vcdbg: Increase boundary to work with newer glibc #27

Closed popcornmix closed 1 year ago

popcornmix commented 1 year ago

Reported by HiassofT

vclog -m is dying with a bus error on RPi4 / LE master since we updated glibc to 2.37. glibc switched to simd optimized memcpy https://sourceware.org/git/?p=glibc.git;a=commit;h=e6f3fe362f1aab78b1448d69ecdbd9e3872636d3 and for whatever reason it doesn't seem to be happy with the 128bit access to the 64bit aligned address from your mmap'ed memory region - is that maybe iomem? (D_q is a define for q3, src a define for x1

Program received signal SIGBUS, Bus error. __memcpy_generic () at ../sysdeps/aarch64/multiarch/../memcpy.S:155 155 ldr D_q, [src] (gdb) where dest@entry=0x7fadcd4014 "\305\300 \001", src=0x7fadd54168 "", src@entry=0x7fadd54164 "\305\300 \001", n=, n@entry=376444) at /home/hias/libreelec/libreelec-master/build.LibreELEC-RPi4.aarch64-12.0-devel/build/bcm2835-utils-5db3e694d27431403bebb13404f5d85899b571e0/vclog/vcdbg.c:742 at /home/hias/libreelec/libreelec-master/build.LibreELEC-RPi4.aarch64-12.0-devel/build/bcm2835-utils-5db3e694d27431403bebb13404f5d85899b571e0/vclog/vcdbg.c:267 (gdb) up dest@entry=0x7fadcd4014 "\305\300 \001", src=0x7fadd54168 "", src@entry=0x7fadd54164 "\305\300 \001", n=, n@entry=376444) at /home/hias/libreelec/libreelec-master/build.LibreELEC-RPi4.aarch64-12.0-devel/build/bcm2835-utils-5db3e694d27431403bebb13404f5d85899b571e0/vclog/vcdbg.c:742 742 memcpy(dest, src, bytes_to_memcpy); (gdb) p dest $1 = 0x7fadcd4018 "" (gdb) p src $2 = 0x7fadd54168 "" (gdb) p bytes_to_memcpy $3 = 376440 (gdb) down 155 ldr D_q, [src] (gdb) info registers x0 0x7fadcd4018 548376756248 x1 0x7fadd54168 548377280872

popcornmix commented 1 year ago

@hiassoft are you happy with this version?

HiassofT commented 1 year ago

It initially looked fine with glibc and vclog debug builds, but I just got a bus error with optimized release builds.

I'll try to find out what's going wrong

popcornmix commented 1 year ago

@HiassofT updated to add no-tree-loop-distribute-patterns

pelwell commented 1 year ago

Isn't there an attribute one can set on the pointer - possibly "packed" - that prevents the optimisations more selectively?

pelwell commented 1 year ago

Not that this is bad.

HiassofT commented 1 year ago

Declaring src parameter of memcpy_vc_memory as const volatile char * seems to help, too.

As it's basically "io memory" declaring it as volatile is also somewhat sensible.

Not's sure though if that really guarantees that compilers would never try optimize any accesses to that region (IIRC they should, but I'm not up-to-date with latest C standards)

popcornmix commented 1 year ago

Isn't there an attribute one can set on the pointer - possibly "packed" - that prevents the optimisations more selectively?

Can pointers be packed? I've only seen it used on structures. Adding __attribute__((packed)) to a pointer results in:

warning: ‘packed’ attribute ignored for type ‘char *’

I'm not sure volatile is quite right (it really means contents pointed may vary without compiler's knowledge), but it probably works in the sense of it makes gcc try fewer optimisations.

asm(""); in the vicinity is likely to work in a similar manner.

I think what we'd like to express is "don't use wider than usual memory accesses", but I'm not sure if that is expressible.

HiassofT commented 1 year ago

As videocore has control over that memory region and can change it at any time, even while we are reading from it, I think volatile is fitting quite well

popcornmix commented 1 year ago

@pelwell are you happy with switching to volatile, compared to other options?

pelwell commented 1 year ago

Yes, if it works - it feels less magic.

popcornmix commented 1 year ago

Updated to volatile version.