search

SDL-Hercules-390 / hyperion

The SDL Hercules 4.x Hyperion version of the System/370, ESA/390, and z/Architecture Emulator
Other
246 stars 92 forks source link

Fix remaining warnings #574

Closed Rhialto closed 1 year ago

Rhialto commented 1 year ago

I put every type of warning in a different commit, together with the actual warning message, so it should be clear what is what, and it can be cherry-picked if desired. With these patches, there are no compiler warnings any more on my system (except the getrusage one from #573). (There are some linker warnings about "/usr/bin/ld: /usr/lib/libc.so and /usr/lib/crt0.o: warning: multiple common of environ" but I suspect that may be too NetBSD specific, and in any case I didn't look into it yet)

wrljet commented 1 year ago

I've run this through a few of my "usual suspects" systems.

Compiles clean with no warnings! on Debian 11 x86_64, gcc 10.2.1 Compiles clean with no warnings! on Raspberry Pi 4B aarch64, gcc 10.2.1 Warnings on Debian 10, 32-bit, x86, gcc 8.3.0 Warnings on macOS Ventura, x86_64, Clang 14.0.0

(logs attached for your enjoyment)

wrljet commented 1 year ago

By the way, "Excellent Work!' on this, and a big thank-you for looking into it.

Bill

Fish-Git commented 1 year ago

Builds cleanly with VS2008 on Windows, for both 32-bit and 64-bit.

64-bit builds cleanly on 64-bit Kubuntu 21.10 with clang 13.0.0-2. Using gcc 11.2.0 however, throws the following [completely bogus IMHO] warnings: *`()`**

In file included from dasdload.c:31:
dasdload2.h: In function ‘process_iebcopy_file.constprop’:
dasdload2.h:2206:25: warning: array subscript ‘DATABLK[0]’ is partly outside array bounds of ‘unsigned char[276]’ [-Warray-bounds]
dasdload2.h:2181:22: note: referencing an object of size 276 allocated by ‘malloc’
In file included from dasdload64.c:31:
dasdload2.h: In function ‘process_iebcopy_file.constprop’:
dasdload2.h:2206:25: warning: array subscript ‘DATABLK[0]’ is partly outside array bounds of ‘unsigned char[276]’ [-Warray-bounds]
dasdload2.h:2181:22: note: referencing an object of size 276 allocated by ‘malloc’

  Bill wrote:

Warnings on Debian 10, 32-bit, x86, gcc 8.3.0 Warnings on macOS Ventura, x86_64, Clang 14.0.0

Q: Is this anything that you and/or anyone else feels should preclude us from accepting Rhialto's PR? Because IMO it shouldn't be. IMO it should be accepted as-is. Does anyone have any real objections to that?

p.s. I too, like Bill, would like to THANK YOU for your effort, Rhialto! Well done! Writing portable code that compiles and runs cleanly on different platforms with different compilers isn't an easy task!

*`()`**  Which of course is not being caused by any of your changes. I only mention it because it does prevent Hercules from compiling cleanly with gcc 11.

wrljet commented 1 year ago

Q: Is this anything that you and/or anyone else feels should preclude us from accepting Rhialto's PR? Because IMO it shouldn't be. IMO it should be accepted as-is. Does anyone have any real objections to that?

I'm just throwing it out as additional testing.

I'm very pleased with the work!

Fish-Git commented 1 year ago

Bill, have you tried VS2022 yet? I'm in the process of trying to do that, but I haven't brought up my VM in so longer, it's taking longer than expected. (Windows is a PIG!)

wrljet commented 1 year ago

Bill, have you tried VS2022 yet? I'm in the process of trying to do that, but I haven't brought up my VM in so longer, it's taking longer than expected. (Windows is a PIG!)

No, I haven't. But I'll do so right now. Stand by...

wrljet commented 1 year ago

Builds with no warnings on VS2022, and passes the canned tests.

Fish-Git commented 1 year ago

VS2022 builds and runs clean for me too.

Any objections to accepting and merging this PR? Speak now or forever hold your peace!

(I'll wait until tomorrow to do the merge just to give others time to respond, should they wish to do so.)

jmaynard commented 1 year ago

The fix for the TID reading in hthreads.c also needs to be applied to hRexx.c. (I've been squashing this same warning, and like Rhialto's solution so much I'm glomming it wholesale.)

Rhialto commented 1 year ago

@jmaynard I guess the compiler didn't warn about the case on line 957 of hRexx.c because I don't have REXX enabled. I will add a commit to the pull request that is similar to the earlier one.

Indeed it seems that later gcc versions have more warnings; gcc 10 is just the one I use for everything on my current system.

wrljet commented 1 year ago

VS2022 builds and runs clean for me too.

Any objections to accepting and merging this PR? Speak now or forever hold your peace!

(I'll wait until tomorrow to do the merge just to give others time to respond, should they wish to do so.)

I'm pleased we got it!