Page MenuHomeFreeBSD

ping: fix alignment errors
ClosedPublic

Authored by sucanjan_gmail.com on Mon, Aug 19, 6:45 PM.

Details

Summary

Fix alignment errors

This fixes -Wcast-align errors when compiled with WARNS=6.

Submitted by: Ján Sučan <sucanjan@gmail.com>
Sponsored by: Google LLC (Google Summer of Code 2019)

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

This change breaks the build on powerpc, powerpc64, and sparc64:

/home/asomers/freebsd/base/head/sbin/ping/ping.c: In function 'pr_pack':
/home/asomers/freebsd/base/head/sbin/ping/ping.c:1144: warning: signed and unsigned type in conditional expression

If the line 1144 is this line

if (icp.icmp_type == icmp_type_rsp) {

then it's interesting because both of the variables are u_char. Could you please provide me more info from the compiler?

That's the whole of the compiler's error output. If you want to reproduce it locally, you should be able to do it with env ARCH=powerpc TARGET_ARCH=powerpc make buildworld. I recommend adding -DNO_CLEAN if you do it more than once. And don't forget the appropriate -j X, or you'll be waiting all day.

Fix the warning about signed and unsigned type in conditional
expression.

asomers added inline comments.Tue, Aug 20, 4:17 PM
sbin/ping/ping.c
1144 ↗(On Diff #61031)

Would this be more clearly written MIN((int)sizeof(icp), cc)?
And since cc is initialized from the output of recvmsg, shouldn't it be of type ssize_t?

sbin/ping/ping.c
1144 ↗(On Diff #61031)

And since cc is initialized from the output of recvmsg, shouldn't it be of type ssize_t?

That's true. Current use of data types in the code is less appropriate than it should be. I will try to fix this in another diff.

Use MIN() instead of a custom conditional expression.

sbin/ping/ping.c
1144 ↗(On Diff #61031)

After doing some experiments it seems that the least error prone way would be to type keep the type casting in (int)sizeof(icp). Changing data type of the variable for recvmsg's return value is OK. The problem is with cc argument to pr_pack. Changing it from int to ssize_t doesn't solve warning about comparing integers with different signs. Changing it from int to size_t (it's a size of a data buffer) starts avalanche of warnings about different signs. If one is corrected, another two appear. Better time for fixing this would be when the code is much more structured so it's much easier to see whether a variable really needs to hold negative values or not.

asomers added inline comments.Wed, Aug 21, 2:35 PM
sbin/ping/ping.c
1144 ↗(On Diff #61031)

True, a size cast would be needed if cc were ssize_t. But that's still better than the current situation which involves both a sign cast and a size cast (implicitly, when assigning from recvmsg).

sbin/ping/ping.c
1144 ↗(On Diff #61031)

Can I change the data type in this diff, or should I create a separate one?

asomers added inline comments.Wed, Aug 21, 8:38 PM
sbin/ping/ping.c
1144 ↗(On Diff #61031)

It should go into this diff. I don't see any reason to postpone.

Change data type of the variable for recvmsg's return value from int to ssize_t.

asomers requested changes to this revision.Fri, Aug 23, 4:27 PM
asomers added inline comments.
sbin/ping/ping.c
898 ↗(On Diff #61089)

I don't think there's any reason to change the type of n, too.

This revision now requires changes to proceed.Fri, Aug 23, 4:27 PM

Data type of variable n for return value of pselect doesn't need to be changed.

sbin/ping/ping.c
898 ↗(On Diff #61089)

Fixed.

This revision needs to be rebased since the changes to -n and -H.

Rebase to the git master branch.

asomers accepted this revision.Fri, Aug 23, 10:02 PM
This revision is now accepted and ready to land.Fri, Aug 23, 10:02 PM
This revision was automatically updated to reflect the committed changes.