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.

Sign up for GitHub

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

Jump to bottom

sys/socket: fix struct sockaddr_storage alignment issue #14595

Merged
merged 1 commit into from
Nov 1, 2024
Merged

sys/socket: fix struct sockaddr_storage alignment issue #14595

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/sys/socket.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ struct sockaddr_storage

/* Following fields are implementation-defined */

struct
struct __attribute__((packed))
Copy link
Contributor

Choose a reason for hiding this comment

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

@nuttxs please replace with begin_packed_struct/end_packed_struct

Copy link
Contributor

Choose a reason for hiding this comment

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

@xiaoxiang781216 what is the advance of begin/end_packed_struct over attribute((packed)) ?

Copy link
Contributor

@xiaoxiang781216 xiaoxiang781216 Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

__attribute__((packed)) is gcc specific extension, other compiler(e.g. msvc) doesn't support it at all. @lupyuen could we restore some Windows ci now?

Copy link
Member

Choose a reason for hiding this comment

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

@xiaoxiang781216 I'm already running the Mirrored Builds twice every day, why don't we use it? https://github.com/NuttX/nuttx/actions/runs/11630100298

@nuttxs To verify the Windows Build on your own NuttX Repo, please follow the steps in "To Run Windows Jobs" thanks!

Copy link
Contributor

@xiaoxiang781216 xiaoxiang781216 Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@xiaoxiang781216 I'm already running the Mirrored Builds twice every day, why don't we use it? https://github.com/NuttX/nuttx/actions/runs/11630100298

@nuttxs To verify the Windows Build on your own NuttX Repo, please follow the steps in "To Run Windows Jobs" thanks!

But it's too late, it's better to catch the error before merging. Please restore one macOS, Windows and msys2 to ci if we have the free quota. If no free quota is available, it's better to remove some Ubuntu ARM build from CI to bring back them.

Copy link
Member

Choose a reason for hiding this comment

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

@xiaoxiang781216 Let's discuss here thanks!

Copy link
Contributor Author

@nuttxs nuttxs Nov 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@xiaoxiang781216
My apologies for the late response. I noticed that you have already submitted a pull request (PR#14608) addressing the compatibility of __attribute__((packed)) for MSVC.

By the way, does MSVC support the __attribute__((aligned(n))) attribute or keyword? I couldn't find the corresponding definition in compiler.h, and Nuttx makes extensive use of __attribute__((aligned(n))) for alignment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lupyuen
I will refer to the guide (#14407) process description. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @nuttxs! We have a newer guide here:

{
char ss_pad1[SS_PAD1SIZE]; /* 6-byte pad; this is to make implementation-defined
* pad up to alignment field that follows explicit in
Expand Down
Loading