Array subscript type warning

[szaydel]

Problem

I do not know if this is seen elsewhere, have not tested, but I do see it on a recent OmniOS version. This issue should exist anywhere, I would expect. The concern here is that char is signed. It is straight-forward to silence this with casting the curSN and newSN to unsigned char, but that's more lipstick than anything.

[27/116] Compiling C object subprojects/opensea-transport/libopensea-transport.a.p/src_scsi_helper.c.o
../subprojects/opensea-transport/src/scsi_helper.c: In function 'seagate_Serial_Number_Cleanup':
../subprojects/opensea-transport/src/scsi_helper.c:2478:44: warning: array subscript has type 'char' [-Wchar-subscripts]
 2478 |                     if ((*unitSerialNumber)[curSN] != '\0')
      |                                            ^
../subprojects/opensea-transport/src/scsi_helper.c:2480:40: warning: array subscript has type 'char' [-Wchar-subscripts]
 2480 |                         newSerialNumber[newSN] = (*unitSerialNumber)[curSN];
      |                                        ^
../subprojects/opensea-transport/src/scsi_helper.c:2480:69: warning: array subscript has type 'char' [-Wchar-subscripts]
 2480 |                         newSerialNumber[newSN] = (*unitSerialNumber)[curSN];
      |                                                                     ^

Expected behavior

I would not expect this compiler warning to be present.

How to reproduce

Just run the build according to the instructions in BUILDING.md on OmniOS.

Deployment information

SunOS omni-lab 5.11 omnios-r151040-d70a3d4f10 i86pc i386 i86pc gcc (OmniOS 151040/11.2.0-il-0) 11.2.0

Additional information

No response

[vonericsen]

Hi @szaydel,

Thanks for reporting this. It is interesting that I have gcc 11.3 on an Ubuntu machine and do not see this warning at all. I launched an omniOS VM and it does repeat this warning. I even tried manually turning on this warning in my Ubuntu machine and it still does not show.

I have seen this happen with GCC on Illumos a few years ago emitting different warnings for the same version, and this seems very similar to that. I have also seen this with cross compilers on Linux, so they probably have a slightly different configuration or way that they are finding these issues and emit the warnings. The old Illumos compiler also didn't understand the #pragma pack instructions that the GCC on linux understood (for compatibility with MSVC) and I had to rewrite the packing instructions on the structures that needed it at the time and I had to go rewrite all of that to eliminate packing in most places.

I do agree this is a good warning to resolve properly so I will figure out a better way to write that section or clean it up in some way since I can at least repeat it in my onmiOS VM.

[szaydel]

Good day @vonericsen! Happy to create more work for you. :) It is certainly curious that on recent Ubuntu you aren't seeing this. I could have bet money that it should have appeared. I suppose it is good to expose this somewhere. With my flawed human eyes it is hard to see how this block of code could fail in its current form, but I think it would be nice to avoid a signed value. Casting just puts a rug over the problem.

[vonericsen]

@szaydel, I've seen weird things like this that you would expect to repeat....but it doesn't always. I don't know exactly why some things are only detected by some versions on some systems, but not others.

I recently setup a cross-compiler for AArch64 on Linux and it spit out warnings I did not see in the x86_64 version of the same compiler...so it does happen, although not often. Cross-CPU compilation is more common to find differences since different CPUs may access some memory or structures slightly differently. I was surprised that x86_64 Linux and x86_64 omniOS showed different output though. This is much less common to find...I think this is only the second time I've seen something like this happen with the same compiler flags/options and the same or nearly the same version of the compiler.

The commit I pushed changed everything to unsigned values and changed the loop condition a little and added a break to exit before doing an unsigned zero minus one, so it should be safe too. I agree that the form generating this error should not have created a bug as it would exit when curSN hit a negative value, but sometimes it's better to just find a different way of writing it to properly satisfy the warning message than just casting away and forgetting about it. If you think that last commit was good, feel free to close this issue, otherwise let me know and I'll see what else I can do to make this better.

[vonericsen]

I also added the -Wchar-subscripts to the meson warning list given to GCC and Clang so if for whatever reason the meson warning level is missing this, it should now be on without question. It's a good warning to have, so it made sense to add it in there as well.

[szaydel]

Sounds great @vonericsen. I guess we live and learn. I agree about the cross-CPU compilation. If this was the case here I too would be less surprised. Nonetheless, I am glad we spotted this and it is one less thing that will have room to go sideways now. And, this change looks a lot nicer and cleaner. And, fits on my screen far better than it did before (another bonus). :)

Thank you very much for jumping on this one!

Cheers!