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

Fix an incorrectly aligned result when converting between pointer types #457

Merged
merged 7 commits into from
Nov 10, 2018

Conversation

derselbst
Copy link
Member

Clang-3.x raises the following warning:

https://travis-ci.org/FluidSynth/fluidsynth/jobs/450156015#L991

This is because the char idlist[] is accessed like a 32bit integer, but as being a character array, it may not be aligned as such. This results in undefined behaviour. To fix it I put it in a union along with a uint32 telling the compiler to suitably align it for integer access. Now, the warning is gone:

https://travis-ci.org/FluidSynth/fluidsynth/jobs/448425040#L841

@mawe42 Do you agree with this fix?

@derselbst derselbst added the bug label Nov 3, 2018
@derselbst derselbst added this to the 2.0-post milestone Nov 3, 2018
@derselbst derselbst requested a review from mawe42 November 3, 2018 12:28
@mawe42
Copy link
Member

mawe42 commented Nov 4, 2018

That looks good! Nifty trick to use a union with a 4-byte integer to force proper alignment. But maybe it would be good to add a short comment above the union to explain it's purpose?

@jjceresa
Copy link
Collaborator

jjceresa commented Nov 6, 2018

Cannot we use "const char c[ ]" instead of "const char c[116]" ?

@derselbst
Copy link
Member Author

derselbst commented Nov 6, 2018

Sure, comment added.

Cannot we use "const char c[ ]" instead of "const char c[116]" ?

No. In C89, arrays wraped in unions must have a fixed size. Otherwise the size of the union would depend on the initialization of its first member, which results in different sizes for different instances of the same union type.

Copy link
Member

@mawe42 mawe42 left a comment

Choose a reason for hiding this comment

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

Very good comment, thanks. Two small typos noted.

};
/*
* This declares a char array containing the SF2 chunk identifiers. This
* array is being access like an uint32 below to simplify id comparision.
Copy link
Member

Choose a reason for hiding this comment

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

accessed

Copy link
Member

Choose a reason for hiding this comment

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

comparison

@jjceresa
Copy link
Collaborator

jjceresa commented Nov 6, 2018

Otherwise the size of the union would depend on the initialization of its first member, which results in different sizes for different instances of the same union type.

Oh, thanks , I had forgotten for this !

Next is out of subject: Also, sorry for the absence of "permanent link" , "quote" and "formatting" message. Since today I cannot use these possibilities anymore (these are now disabled). Guys at GitHubSoldOffByMicrosoft now have complicated things. Here GitHub web page was notified requesting for upgrading another recent web browser or OS !.

"A member of a const-qualified structure or union type acquires the qualification of the type it belongs to." I.e. if the whole union is const, all its members are const.
@derselbst
Copy link
Member Author

No problem jjc :)

@derselbst derselbst merged commit 06bcf8d into master Nov 10, 2018
@derselbst derselbst deleted the idlist branch November 10, 2018 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants