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

build: add --enable-optimization-warnings #4166

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

asadsa92
Copy link
Contributor

Start off by adding -Wpadded and -Wno-error=padded . This is useful when investigating holes in the struct layout. Alternatively, the pahole utility can be used for this.

Dridi suggested that I add this to a new class of warnings.

Start off by adding -Wpadded and -Wno-error=padded .
This is useful when investigating holes in the struct layout.
Alternatively, the pahole utility can be used for this.

Dridi suggested that I add this to a new class of warnings.

Signed-off-by: Asad Sajjad Ahmed <[email protected]>
@asadsa92 asadsa92 self-assigned this Aug 20, 2024
@bsdphk
Copy link
Contributor

bsdphk commented Aug 26, 2024

I am not entirely on board with this, because there are valid reasons not want structs tightly packed, for instance cacheline considerations.

It also bothers me that the patch calls them "OPTIMIZATION_FLAGS", they're not. At best they're "DEVELOPER_INFORMATION_FLAGS" or something of that sort.

@asadsa92
Copy link
Contributor Author

I am not entirely on board with this, because there are valid reasons not want structs tightly packed, for instance cacheline considerations.

Yes, we can always ignore false-positives. The intend was not to enable this by default.
We could also place a reserved variable there to fill the hole and make it more explicit.
Then, in the future, we can use this space for something else rather than expanding the struct size.

It also bothers me that the patch calls them "OPTIMIZATION_FLAGS", they're not. At best they're "DEVELOPER_INFORMATION_FLAGS" or something of that sort.

Sure, we can rename it.
I do not think it make sense to enable this through the default ./autogen.des invocation, at least not before we have reviewed what we got first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants