eclipse / tinydtls

Eclipse tinydtls
https://projects.eclipse.org/projects/iot.tinydtls
Other
105 stars 57 forks source link

tests: Fix compiler clash over index usage in test suites #194

Closed mrdeep1 closed 1 year ago

mrdeep1 commented 1 year ago

Some compilers complain that index is already defined in . Replace index with lindex.

boaks commented 1 year ago

For me this looks more like we should check, if we can get rid of "#include ". I only see, it's used in "ccm-test.c" and I miss to see, why it is required. Maybe you can help me to see, where "dtls-client" refers to "strings.h"?

mrdeep1 commented 1 year ago

My mistake - it should have been <string.h>. I get the following output

dtls_ciphers_util.c: In function 'find_cipher_suite':
dtls_ciphers_util.c:47: warning: declaration of 'index' shadows a global declaration
/usr/include/string.h:489: warning: shadowed declaration is here
dtls_ciphers_util.c: In function 'add_cipher_suite':
dtls_ciphers_util.c:60: warning: declaration of 'index' shadows a global declaration
/usr/include/string.h:489: warning: shadowed declaration is here
dtls_ciphers_util.c: In function 'cipher_suites_usage':
dtls_ciphers_util.c:91: warning: declaration of 'index' shadows a global declaration
/usr/include/string.h:489: warning: shadowed declaration is here
dtls-server.c: In function 'get_user_parameter':
dtls-server.c:203: warning: declaration of 'index' shadows a global declaration
/usr/include/string.h:489: warning: shadowed declaration is here
dtls-client.c: In function 'get_user_parameters':
dtls-client.c:242: warning: declaration of 'index' shadows a global declaration
/usr/include/string.h:489: warning: shadowed declaration is here
boaks commented 1 year ago

Hm, strange. I'm not that common to C. But for me it looks, like that "string.h" is out of standard, or? AFAIK "strings.h" includes that "index", but that "string.h" also includes it, makes me wonder. On which platform had you recognized that?

mrdeep1 commented 1 year ago

I saw it on CentOS 6, which I agree is now somewhat old. The likes of ./configure and cmakecheck for both and

boaks commented 1 year ago

The likes of ./configure and cmake check for both and

My first idea was to remove the autoconfig/cmake support for . Otherwise, if that doesn't help and we need to consider "index" as reserved identifier, I would replace it then by a simple "i".

mrdeep1 commented 1 year ago

Replaced with a simple "i" and changes pushed.

boaks commented 1 year ago

"index" is also used by dtls.c. Do you get a warning also there?

mrdeep1 commented 1 year ago

No - but I guess that is down to neither <string.h> or <strings.h> being (directly) included. That said, dtls.c includes session.h includes <string.h>. Hmm

mrdeep1 commented 1 year ago

OK - issue is that dtls.cis not compiled with -D_GNU_SOURCE, but the tests are, which throws up the indexconflict.

boaks commented 1 year ago

Maybe we remove _GNU_SOURCE and see what happens? Or just go with "i"?

mrdeep1 commented 1 year ago

Removal of _GNU_SOURCE is a non-starter - even in Ubuntu-20. The Ubuntu-20 compile errors are many, starting with

dtls-server.c: In function ‘dtls_handle_signal’:
dtls-server.c:255:3: warning: implicit declaration of function ‘kill’ [-Wimplicit-functiondeclaration]
  255 |   kill(getpid(), sig);
      |   ^~~~
dtls-server.c:255:3: warning: nested extern declaration of ‘kill’ [-Wnested-externs]
dtls-server.c: In function ‘resolve_address’:
dtls-server.c:262:19: error: storage size of ‘hints’ isn’t known
  262 |   struct addrinfo hints;
      |                   ^~~~~
dtls-server.c:276:11: warning: implicit declaration of function ‘getaddrinfo’ [-Wimplicit-unction-declaration]
  276 |   error = getaddrinfo(addrstr, NULL, &hints, &res);
      |           ^~~~~~~~~~~
dtls-server.c:276:11: warning: nested extern declaration of ‘getaddrinfo’ [-Wnested-extern]
....
boaks commented 1 year ago

LGTM

obgm commented 1 year ago

Removal of _GNU_SOURCE is a non-starter - even in Ubuntu-20. The Ubuntu-20 compile errors are many, starting with

kill() and getaddrinfo() are POSIX, so setting _POSIX_C_SOURCE instead of _GNU_SOURCE should do (for these, anyway). The less "proprietary" we are the better. While this is a general rule, I also agree with renaming the variable to avoid the name clash. (And keep wondering why on earth they put a variable index in a global namespace....)

The replacement would look like this. A bit ugly as well...: replace-gnu-source-with-posix.patch

mrdeep1 commented 1 year ago

I'm happy to add in replace-gnu-source-with-posix.patch to this PR, or should it be in a new PR? Use of index still needs to be fixed.

obgm commented 1 year ago

For the _GNU_SOURCE → _POSIX_C_SOURCE, I would go for a separate PR.