search

ibm-s390-linux / s390-tools

Tools for use with the s390 Linux kernel and device drivers
MIT License
63 stars 60 forks source link

fix some warnings #137

Closed sharkcz closed 2 years ago

sharkcz commented 2 years ago
sharkcz commented 2 years ago

I think I have finished :-) What remains are a number of "array bounds" warnings in zipl and 2 warnings for unaligned address of a member in packed struct (mon_procd.c and zfcpdump_part.c).

sharkcz commented 2 years ago

My environment for this exercise is Fedora 36 with GCC 12 and building from git, meaning the project's compiler flags have been used.

hoeppnerj commented 2 years ago

Thanks a lot for the patches @sharkcz . I'm aware of these warnings but haven't come around addressing any of those. Due to vacation time though it'll only be possible to start reviewing the patches in ~2 weeks.

sharkcz commented 2 years ago

As far as ziomon goes I don't see a problem with the change so far. Even if we get unaligned accesses, this shouldn't be a functional problem on s390x, x86(_64), or arm64.

Thanks for review. Could you ask your toolchain team for an opinion too? Perhaps GCC is too aggressive, because the warning is rather performance related than functionally IMO.

Benjamin-Block commented 2 years ago

As far as ziomon goes I don't see a problem with the change so far. Even if we get unaligned accesses, this shouldn't be a functional problem on s390x, x86(_64), or arm64.

Thanks for review. Could you ask your toolchain team for an opinion too? Perhaps GCC is too aggressive, because the warning is rather performance related than functionally IMO.

So are you suggesting this got worse with new GCC versions because it now tries to optimize but makes performance worse? Instead of it does the same as before, but now issues a warning?

Because so far I'm not aware of reports of performance issues with ziomon - IOW if the performance doesn't get worse now due to ill-performed optimization attempts by GCC compare to previous versions, I don't see a big issue; although, improving this by reducing potential unaligned accesses is a good idea, it's just not easily doable with the ziomon's design right now.

@Andreas-Krebbel @iii-i @rdapp1

sharkcz commented 2 years ago

I mean the unaligned access might have negative performance effects on some platforms (not the case of s390x). The warning has been reported in the build for quite some time already.

Also would be good to know what is the reason for the "unaligned access" warning in the case of s390x, where the access should just work without any negative impact (I guess). I agree avoiding the unaligned accesses could be difficult.

Benjamin-Block commented 2 years ago

I mean the unaligned access might have negative performance effects on some platforms (not the case of s390x). The warning has been reported in the build for quite some time already.

Fair enough. Although AFAIK the effect is not 0, not even on s390x; the question is, do we have a problem with that in ziomon.

In general ziomon is by design split in two parts (http://www.vm.ibm.com/education/lvc/LVC0425.pdf): (a) ziomon_* that records data during the operation on the target machine and writes it into files; (b) ziorep_* utilities that take the recordings and generate various reports from them. (a) will only be ever run on s390x, since it requires FICON Express adapters. Performance could be relevant here, since recording data should in best case not take too much CPU time so we don't disturb the data (too much). (b) might also be run on other platforms - e.g. a developer/operator laptop (x86_64 or maybe now arm64). Performance is not really relevant here, since this is essentially post-processing (although of course, it would be good if this isn't too slow).

Bottom line is, the part where performance might be of interest will only run on s390x. I don't know about performance issues here right now. Improvements are probably possible though.

Also would be good to know what is the reason for the "unaligned access" warning in the case of s390x, where the access should just work without any negative impact (I guess). I agree avoiding the unaligned accesses could be difficult.

Like I said, I don't know that the (performance) effect is really 0 on s390x; just that so far I don't know about a problem with it in the context of ziomon.

The warnings seems to be architecture independent AFAICT - it's called from the compiler front end - and is always enabled by default for C/C++.

Andreas-Krebbel commented 2 years ago

I would say as a general rule never use "packed" if there isn't a really good reason to do that. The "packed" attribute is a language extensions which violates the ABI. The only valid reason to use it is if you have to match hardware provided data structures. This doesn't appear to be the case here:

struct message {
        __u32   length; /* length of the message,
                           excluding the 'length' and 'type' attributes.
                           Or, in other word, length of 'data' */
        __u32   type;
        void   *data;
} __attribute__ ((packed));

Here the "packed" attribute doesn't even help with getting rid of padding. The only affect is that the whole struct gets an alignment of 1 and that's most certainly neither required nor wanted.

I would recommend revisiting all the "packed" struct definitions. If "packed" isn't mandated because the struct is given that way by the hardware or a kernel interface just drop it. If "packed" has been added to get rid of padding, I would rather try to rearrange the struct members to get rid of it.

The GCC warning is correct here since you are creating a misaligned pointer which is undefined behavior: C99 6.3.2.3 §7 A pointer to an object or incomplete type may be converted to a pointer to a different object or incomplete type. If the resulting pointer is not correctly aligned for the pointed-to type, the behavior is undefined. ...

Benjamin-Block commented 2 years ago

Thanks for the comment Andreas.

I would say as a general rule never use "packed" if there isn't a really good reason to do that. The "packed" attribute is a language extensions which violates the ABI. The only valid reason to use it is if you have to match hardware provided data structures. This doesn't appear to be the case here:

Agreed, I don't know the exact history why so many structures in ziomon are declared as 'packed'. There is some interfaces with the kernel, but this supposedly doesn't affect all of these structures.

struct message {
        __u32   length; /* length of the message,
                           excluding the 'length' and 'type' attributes.
                           Or, in other word, length of 'data' */
        __u32   type;
        void   *data;
} __attribute__ ((packed));

Here the "packed" attribute doesn't even help with getting rid of padding. The only affect is that the whole struct gets an alignment of 1 and that's most certainly neither required nor wanted.

Yeah, agreed, I saw that as well. No idea why this was declared like that.

I would recommend revisiting all the "packed" struct definitions. If "packed" isn't mandated because the struct is given that way by the hardware or a kernel interface just drop it. If "packed" has been added to get rid of padding, I would rather try to rearrange the struct members to get rid of it.

This is my "long-term" wish. If we had a proper regression test suite for ziomon, we could try and phase out these declarations where necessary and be relatively confident it doesn't break. But that is nothing we can possibly do short-term. If @sharkcz and @hoeppnerj want to get rid of the warnings because it breaks build-systems in distributions or something like that the short-term solution is this patch I (still) think - the Linux kernel has this also per default: https://git.kernel.org/linus/6f303d60534c.

It's certainly not ideal, but if this is too controversial, then better take the patch out of the series, and life with the warnings.

The GCC warning is correct here since you are creating a misaligned pointer which is undefined behavior: C99 6.3.2.3 §7 A pointer to an object or incomplete type may be converted to a pointer to a different object or incomplete type. If the resulting pointer is not correctly aligned for the pointed-to type, the behavior is undefined. ...

So I take this as, the purpose of the Warning is really, this might break, if wrongly used and/or wrongly optimized. And that doesn't have anything to do with the architecture, but with the C standard.

sharkcz commented 2 years ago

I would personally prefer to disable the warning, it doesn't break anything, but it makes very difficult to notice any additional warnings for ziomon.

hoeppnerj commented 2 years ago

@Benjamin-Block @sharkcz I'm fine with the warnings being disabled for now. However, it'd be nice to have the removal of __packed (as Andreas suggested) somehow put on your (team's) TODO lists so it won't get lost.

sharkcz commented 2 years ago

@Benjamin-Block @sharkcz I'm fine with the warnings being disabled for now. However, it'd be nice to have the removal of __packed (as Andreas suggested) somehow put on your (team's) TODO lists so it won't get lost.

from my point of view opening a new issue here would be best

hoeppnerj commented 2 years ago

@Benjamin-Block @sharkcz I'm fine with the warnings being disabled for now. However, it'd be nice to have the removal of __packed (as Andreas suggested) somehow put on your (team's) TODO lists so it won't get lost.

from my point of view opening a new issue here would be best

Makes perfect sense, thanks for doing it!

sharkcz commented 2 years ago

rebase to master and applied @hoeppnerj 's suggestion

sharkcz commented 2 years ago

Thanks :-)