intel / ledmon

Enclosure LED Utilities
GNU General Public License v2.0
72 stars 49 forks source link

[BUG]: Failure to build on 32bit #237

Closed tasleson closed 3 months ago

tasleson commented 4 months ago

Description

Using a 32bit gcc tool chain I'm running into compiler error and warnings

Steps to reproduce bug

Compile using 32bit tool chain e.g.

$ CFLAGS="-m32 -march=i686" ./configure --enable-library
$ make

Expected behavior

Clean compile

Actual behavior

Compile error

utils.c: In function 'get_uint64':
utils.c:101:18: error: passing argument 1 of 'str_toul' from incompatible pointer type [-Wincompatible-pointer-types]
  101 |         str_toul(&defval, p, NULL, 16);
      |                  ^~~~~~~
      |                  |
      |                  uint64_t * {aka long long unsigned int *}
In file included from utils.c:31:
utils.h:395:29: note: expected 'long unsigned int *' but argument is of type 'uint64_t *' {aka 'long long unsigned int *'}
  395 | int str_toul(unsigned long *dest, const char *strptr, char **endptr, int base);
      |              ~~~~~~~~~~~~~~~^~~~

Possible fix for error:

diff --git a/src/lib/utils.c b/src/lib/utils.c
index a7936c6..ec1067f 100644
--- a/src/lib/utils.c
+++ b/src/lib/utils.c
@@ -98,8 +98,11 @@ uint64_t get_uint64(const char *path, uint64_t defval, const char *name)
        if (!p)
                return defval;

-       str_toul(&defval, p, NULL, 16);
-       return defval;
+       errno = 0;
+       uint64_t t = strtoull(p, NULL, 16);
+       if (errno)
+               return defval;
+       return t;
 }

 int get_int(const char *path, int defval, const char *name)

Compile warnings

 CC       libledinternal_la-amd_sgpio.lo
amd_sgpio.c:37:56: warning: left shift count >= width of type [-Wshift-count-overflow]
   37 |         uint64_t        _##type##_##name##_mask = mask << shift;        \
      |                                                        ^~
amd_sgpio.c:81:1: note: in expansion of macro 'DECLARE_SGPIO'
   81 | DECLARE_SGPIO(req, reg_count, 32, 0xFFL)
      | ^~~~~~~~~~~~~
amd_sgpio.c:37:56: warning: left shift count >= width of type [-Wshift-count-overflow]
   37 |         uint64_t        _##type##_##name##_mask = mask << shift;        \
      |                                                        ^~
amd_sgpio.c:98:1: note: in expansion of macro 'DECLARE_SGPIO'
   98 | DECLARE_SGPIO(cfg, blink_gen_a, 40, 0xFL);
      | ^~~~~~~~~~~~~
amd_sgpio.c:37:56: warning: left shift count >= width of type [-Wshift-count-overflow]
   37 |         uint64_t        _##type##_##name##_mask = mask << shift;        \
      |                                                        ^~
amd_sgpio.c:99:1: note: in expansion of macro 'DECLARE_SGPIO'
   99 | DECLARE_SGPIO(cfg, blink_gen_b, 44, 0xFL);
      | ^~~~~~~~~~~~~
amd_sgpio.c:37:56: warning: left shift count >= width of type [-Wshift-count-overflow]
   37 |         uint64_t        _##type##_##name##_mask = mask << shift;        \
      |                                                        ^~
amd_sgpio.c:100:1: note: in expansion of macro 'DECLARE_SGPIO'
  100 | DECLARE_SGPIO(cfg, max_on, 48, 0xFL);
      | ^~~~~~~~~~~~~
amd_sgpio.c:37:56: warning: left shift count >= width of type [-Wshift-count-overflow]
   37 |         uint64_t        _##type##_##name##_mask = mask << shift;        \
      |                                                        ^~
amd_sgpio.c:101:1: note: in expansion of macro 'DECLARE_SGPIO'
  101 | DECLARE_SGPIO(cfg, force_off, 52, 0xFL);
      | ^~~~~~~~~~~~~
amd_sgpio.c:37:56: warning: left shift count >= width of type [-Wshift-count-overflow]
   37 |         uint64_t        _##type##_##name##_mask = mask << shift;        \
      |                                                        ^~
amd_sgpio.c:102:1: note: in expansion of macro 'DECLARE_SGPIO'
  102 | DECLARE_SGPIO(cfg, stretch_on, 56, 0xFL);
      | ^~~~~~~~~~~~~
amd_sgpio.c:37:56: warning: left shift count >= width of type [-Wshift-count-overflow]
   37 |         uint64_t        _##type##_##name##_mask = mask << shift;        \
      |                                                        ^~
amd_sgpio.c:103:1: note: in expansion of macro 'DECLARE_SGPIO'
  103 | DECLARE_SGPIO(cfg, stretch_off, 60, 0xFL);
      | ^~~~~~~~~~~~~
  CC       libledinternal_la-amd_ipmi.lo

Environment

gcc (GCC) 14.0.1 20240411 (Red Hat 14.0.1-0)

Ledmon version

Latest main branch

Ledmon logs

NA

Ledctl logs

NA

Ledmon supported controllers

NA

Additional information

No response

mtkaczyk commented 4 months ago

Hi @tasleson, Thanks for reporting. Are you going to work on it? Is it just a issue you discovered, or you have 32bits setups you want to work on? I'm asking to determine the real priority.

To be clear 32 bit is not something we are targeting but I'm open to change my mind. In this case it is probably low effort.

tasleson commented 4 months ago

Thanks for reporting. Are you going to work on it?

When I get a chance I can look into this a bit more.

Is it just a issue you discovered, or you have 32bits setups you want to work on?

I had a failure to compile for 32bit on a package I maintain that is using ledmon-lib. We'll want to address this, but I don't think it's super high priority. The get_uint64 looks like a real bug if we run into a string that has an integral value > 32 bit on any arch, I'm not sure at the moment about the shift warnings.

tpetazzoni commented 3 months ago

We are seeing the same build issue while building on MIPS64:

<command-line>: note: this is the location of the previous definition
utils.c: In function 'get_uint64':
utils.c:118:18: error: passing argument 1 of 'str_toul' from incompatible pointer type [-Wincompatible-pointer-types]
  118 |         str_toul(&defval, p, NULL, 16);
      |                  ^~~~~~~
      |                  |
      |                  uint64_t * {aka long long unsigned int *}

See http://autobuild.buildroot.net/results/125/1256ff22e9e5a6f0359e2d6d813d9c511d7f27aa/build-end.log

mtkaczyk commented 3 months ago

I think it is fixed now. Thanks @tasleson!

tasleson commented 3 months ago

I think it is fixed now. Thanks @tasleson!

You're welcome, but I don't see any fixes committed for this, so how is this resolved?

mtkaczyk commented 3 months ago

@tasleson yeah my bad. Now it is fixed :)

mtkaczyk commented 3 months ago

https://github.com/intel/ledmon/pull/245