pkarashchenko commented on a change in pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#discussion_r778319423
##########
File path: include/nuttx/net/usrsock.h
##########
@@ -227,9 +228,9 @@ begin_packed_struct struct usrsock_message_req_ack_s
{
struct usrsock_message_common_s head;
- uint8_t xid;
- uint8_t reserved;
- int32_t result;
+ int16_t reserved;
Review comment:
> We can't leverage the compiler do the native alignment as discuss
before since the different compiler may align to the different place.
Do you have some examples where two different compilers apply different
alignments when compiled for a same target? Is this is a theoretical compiler
or you are referring to compiler options for default alignment? I mean that
compiler can't violate the rules for target architecture and setting default
alignment will possibly make structures smaller but will most probably lead to
generation of the slower code.
> packed will remove any pad from the struct, so if we don't add some
reserved field here, many fields will become unaligned. This will impact the
performance and code size:
> 1. If the hardware doesn't support unaligned access, compiler will split
the bus transaction into small and aligned, which increase code size and
decrease performance both.
> 2. If the hardware does support unaligned access, compiler will issue the
access directly, and hardware will split it into small and aligned if the
access isn't aligned. In this case, only the performance get impact.
That is true but I do not agree with "if we don't add some reserved field
here, many fields will become unaligned" in case of `packed`. Here is a test
code and generated assembly:
test.c
```
struct s1 {
unsigned int a;
} __attribute__((packed));
struct s2 {
unsigned int a;
};
unsigned int f1(struct s1 *s)
{
return s->a;
}
unsigned int f2(struct s2 *s)
{
return s->a;
}
```
test.s
```
.cpu arm7tdmi
.arch armv4t
.fpu softvfp
.eabi_attribute 20, 1
.eabi_attribute 21, 1
.eabi_attribute 23, 3
.eabi_attribute 24, 1
.eabi_attribute 25, 1
.eabi_attribute 26, 1
.eabi_attribute 30, 6
.eabi_attribute 34, 0
.eabi_attribute 18, 4
.file "test.c"
.text
.align 1
.global f1
.syntax unified
.code 16
.thumb_func
.type f1, %function
f1:
@ Function supports interworking.
@ args = 0, pretend = 0, frame = 8
@ frame_needed = 1, uses_anonymous_args = 0
push {r7, lr}
sub sp, sp, #8
add r7, sp, #0
str r0, [r7, #4]
ldr r3, [r7, #4]
ldrb r2, [r3]
ldrb r1, [r3, #1]
lsls r1, r1, #8
orrs r2, r1
ldrb r1, [r3, #2]
lsls r1, r1, #16
orrs r2, r1
ldrb r3, [r3, #3]
lsls r3, r3, #24
orrs r3, r2
movs r0, r3
mov sp, r7
add sp, sp, #8
@ sp needed
pop {r7}
pop {r1}
bx r1
.size f1, .-f1
.align 1
.global f2
.syntax unified
.code 16
.thumb_func
.type f2, %function
f2:
@ Function supports interworking.
@ args = 0, pretend = 0, frame = 8
@ frame_needed = 1, uses_anonymous_args = 0
push {r7, lr}
sub sp, sp, #8
add r7, sp, #0
str r0, [r7, #4]
ldr r3, [r7, #4]
ldr r3, [r3]
movs r0, r3
mov sp, r7
add sp, sp, #8
@ sp needed
pop {r7}
pop {r1}
bx r1
.size f2, .-f2
.ident "GCC: (GNU Arm Embedded Toolchain 10.3-2021.10) 10.3.1 20210824
(release)"
```
As you can see there is no "reserved field", but in case of `packed`
compiler still splits loads into multiple `ldrb`s + `orrs`s to make a value
reassembly. The reason is that `packed` types can be spaced at any address in
memory and even adding a "reserved field" will not lead to a better code
generation.
Sorry for a long discussion, this is because I was looking into the code
from GitHub web view only till now. Now I had an opportunity to checkout the
code and I see what @anchao is trying to do here trying to achieve whole
message best alignment. I was missing the inheritance part for common headers.
Now things are clear to me. Thanks again for spending time explaining to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
