Fix compiler warnings #75

yan-foto · 2020-08-27T16:43:28Z

As discussed in #74 here is a set of changes that fix compiler warnings. Some of the changes are trivial and non-critical while the others fix some hidden bugs such as missing return statements and possibly unsafe pointer arithmetic.

Each commit addresses a specific warning type and additional information (if applicable) is also included in commit message.

Changes fix following errors: - encode/signed-interest.c:298:24: warning: comparison of integer expressions of different signedness: ‘size_t’ {aka ‘unsigned int’} and ‘int’ - encode/signed-interest.c:347:24: warning: comparison of integer expressions of different signedness: ‘size_t’ {aka ‘unsigned int’} and ‘int’ - encode/signed-interest.c:395:24: warning: comparison of integer expressions of different signedness: ‘size_t’ {aka ‘unsigned int’} and ‘int’ - encode/wrapper-api.c:256:17: warning: comparison of integer expressions of different signedness: ‘uint32_t’ {aka ‘unsigned int’} and ‘int’ - encode/wrapper-api.c:670:17: warning: comparison of integer expressions of different signedness: ‘uint32_t’ {aka ‘unsigned int’} and ‘int’ - forwarder/name-tree.c:88:17: warning: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘unsigned int’} - forwarder/name-tree.c:117:17: warning: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘unsigned int’} - forwarder/name-tree.c:168:17: warning: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘unsigned int’} NOTE: some changes are just removal of white spaces (automatically applied by my IDE).

Changes fix following error: - encode/wrapper-api.c:405:12: warning: this statement may fall through [-Wimplicit-fallthrough=]

Changes fix following warnings: - encode/encrypted-payload.c:117:23: warning: format ‘%ld’ expects argument of type ‘long int’, but argument 2 has type ‘uint32_t’ {aka ‘unsigned int’} - encode/encrypted-payload.c:117:23: warning: format ‘%ld’ expects argument of type ‘long int’, but argument 3 has type ‘uint32_t’ {aka ‘unsigned int’}

Changes fix following errors: - security/ndn-lite-hmac.c:146:18: warning: comparison of unsigned expression < 0 is always false (3 errors on the same line)

Changes fix following errors: - encode/key-storage.c:126:1: error: old-style function definition - encode/key-storage.c:156:1: error: old-style function definition

Changes fixes following warnings: - forwarder/forwarder.c:314:1: error: control reaches end of non-void function

Changes fix following warnings: - forwarder/forwarder.c:311:37: error: passing argument 2 of ‘ndn_interest_tlv_encode’ discards ‘const’ qualifier from pointer target type NOTE: the cause of this warning is 'ndn_interest_tlv_encode' in which name components are updated to cater for a SHA256 digest.

Changes fix following warnings: - forwarder/name-tree.c:83:17: error: variable ‘last_node’ set but not used NOTE: this unused variable seems to be remainder of a 'copy/paste' action!

Changes fix following warnings: - encode/name-component.c:30:10: error: 'result' may be used uninitialized in this function [-Werror=maybe-uninitialized] - encode/../util/logger.h:25:3: error: 'keyid' may be used uninitialized in this function NOTE: this error occures only in older compilers that are not smart enough to understand that the uninitialized vale are filled out later.

yan-foto · 2020-08-27T16:54:09Z

I still get a number of unused-parameter warnings which I have postponed for the future. Maybe you guys can take a look at them.

yoursunny · 2020-08-27T17:16:13Z

It’s necessary to have continuous integration that turns on -Werror to ensure these warnings do not reappear.
Warnings that have not been fixed can then be turned off via -Wno-error=unused-parameter.

git-subtree-dir: tests git-subtree-mainline: ae85f84 git-subtree-split: 0a9954d

In previous structure, NDN lite tests were in their own repository with NDN lite sources as a submodule in base directory. Here, the relationship is inverse, i.e., the tests are integrated directly in NDN lite repository. In this commit a really cheap trick (symlink to parent instead of submodule) is used to have the tests running without changing the source code. This might need some reconsideration in the future!

yoursunny · 2020-09-01T21:41:51Z

I tested f0998b4 on Ubuntu 16.04 using GCC 5.4.0.

I'm still seeing warnings (that do not belong in test files). For example:

/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c: In function ‘_on_ac_notification’:
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:141:22: warning: passing argument 1 of ‘ndn_name_print’ from incompatible pointer type [-Wincompatible-pointer-types]
   NDN_LOG_DEBUG_NAME(&notification);
                      ^
/tmp/ndn-lite/tests/ndn-lite/app-support/../util/logger.h:41:18: note: in definition of macro ‘NDN_LOG_DEBUG_NAME’
   ndn_name_print(name);\
                  ^
In file included from /tmp/ndn-lite/tests/ndn-lite/app-support/../encode/interest.h:14:0,
                 from /tmp/ndn-lite/tests/ndn-lite/app-support/access-control.h:14,
                 from /tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:11:
/tmp/ndn-lite/tests/ndn-lite/app-support/../encode/name.h:42:1: note: expected ‘const ndn_name_t * {aka const struct ndn_name *}’ but argument is of type ‘ndn_interest_t * {aka struct ndn_interest *}’
 ndn_name_print(const ndn_name_t* name);
 ^
In file included from /tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:30:0:
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:149:21: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 3 has type ‘uint32_t {aka unsigned int}’ [-Wformat=]
       NDN_LOG_DEBUG("[ACCESSCTL] Enforced update for Service %u, KeyID %lu\n",
                     ^
/tmp/ndn-lite/tests/ndn-lite/app-support/../util/logger.h:38:10: note: in definition of macro ‘NDN_LOG_DEBUG’
   printf(__VA_ARGS__); \
          ^
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c: In function ‘_on_ekey_data’:
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:208:17: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 2 has type ‘uint32_t {aka unsigned int}’ [-Wformat=]
   NDN_LOG_DEBUG("[ACCESSCTL] AES KeyID = %lu \n", keyid);
                 ^
/tmp/ndn-lite/tests/ndn-lite/app-support/../util/logger.h:38:10: note: in definition of macro ‘NDN_LOG_DEBUG’
   printf(__VA_ARGS__); \
          ^
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:253:17: warning: format ‘%llu’ expects argument of type ‘long long unsigned int’, but argument 2 has type ‘ndn_time_us_t {aka long unsigned int}’ [-Wformat=]
   NDN_LOG_DEBUG("[ACCESSCTL] Key update: %lluus\n", m_measure_tp2 - m_measure_t
                 ^
/tmp/ndn-lite/tests/ndn-lite/app-support/../util/logger.h:38:10: note: in definition of macro ‘NDN_LOG_DEBUG’
   printf(__VA_ARGS__); \
          ^
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c: In function ‘_on_dkey_data’:
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:308:17: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 2 has type ‘uint32_t {aka unsigned int}’ [-Wformat=]
   NDN_LOG_DEBUG("[ACCESSCTL] AES KeyID = %lu \n", keyid);
                 ^
/tmp/ndn-lite/tests/ndn-lite/app-support/../util/logger.h:38:10: note: in definition of macro ‘NDN_LOG_DEBUG’
   printf(__VA_ARGS__); \
          ^
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c: In function ‘_express_ekey_interest’:
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:394:35: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
   ndn_name_t* self_identity_key = ndn_key_storage_get_self_identity_key(service
                                   ^
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:399:60: warning: passing argument 3 of ‘ndn_signed_interest_ecdsa_sign’ from incompatible pointer type [-Wincompatible-pointer-types]
   ndn_signed_interest_ecdsa_sign(&interest, self_identity, self_identity_key);
                                                            ^
In file included from /tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:15:0:
/tmp/ndn-lite/tests/ndn-lite/app-support/../encode/signed-interest.h:43:1: note: expected ‘const ndn_ecc_prv_t * {aka const struct ndn_ecc_prv *}’ but argument is of type ‘ndn_name_t * {aka struct ndn_name *}’
 ndn_signed_interest_ecdsa_sign(ndn_interest_t* interest,
 ^
In file included from /tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:30:0:
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c: In function ‘ndn_ac_trigger_expiration’:
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:577:19: warning: format ‘%ld’ expects argument of type ‘long int’, but argument 2 has type ‘uint32_t {aka unsigned int}’ [-Wformat=]
     NDN_LOG_DEBUG("[ACCESSCTL] Local Decryption Key %ld forced expired\n", aes_
                   ^
/tmp/ndn-lite/tests/ndn-lite/app-support/../util/logger.h:38:10: note: in definition of macro ‘NDN_LOG_DEBUG’
   printf(__VA_ARGS__); \
          ^
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:581:19: warning: format ‘%ld’ expects argument of type ‘long int’, but argument 2 has type ‘uint32_t {aka unsigned int}’ [-Wformat=]
     NDN_LOG_DEBUG("[ACCESSCTL] Notifying Encryption Key %ld forced expired\n",
                   ^
/tmp/ndn-lite/tests/ndn-lite/app-support/../util/logger.h:38:10: note: in definition of macro ‘NDN_LOG_DEBUG’
   printf(__VA_ARGS__); \
          ^
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c: In function ‘_on_ac_notification’:
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:158:1: warning: control reaches end of non-void function [-Wreturn-type]
 }
 ^
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c: In function ‘ndn_ac_trigger_expiration’:
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:599:1: warning: control reaches end of non-void function [-Wreturn-type]
 }
 ^

The proper way to resolve -Wformat is to #include <inttypes.h> and then use PRIu64 etc.
For things like ndn_time_us_t, add a #define PRI_ndn_time_us_t PRIu64 right next to the typedef declaring that type, then use PRI_ndn_time_us_t macro when you need to print a value of that type.

yan-foto · 2020-09-01T21:58:11Z

The proper way to resolve -Wformat is to #include <inttypes.h> and then use PRIu64 etc. For things like ndn_time_us_t, add a #define PRI_ndn_time_us_t PRIu64 right next to the typedef declaring that time, then use PRI_ndn_time_us_t macro when you need to print a value of that type. Yes, that also came to my attention after I posted the comment. This is obviously due to my lack of experience in writing portable/cross platform code!Thanks for the hint! I'll take care of it tomorrow.

Changes here make format strings portable and cross platform compatible.

yoursunny

/tmp/ndn-lite/tests/ndn-lite/app-support/security-bootstrapping.c: In function ‘sec_boot_send_cert_interest’:
/tmp/ndn-lite/tests/ndn-lite/app-support/security-bootstrapping.c:286:46: error: passing argument 5 of ‘ndn_forwarder_express_interest’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
                                              &m_sec_boot_state.device_info->ser
                                              ^
In file included from /tmp/ndn-lite/tests/ndn-lite/app-support/security-bootstrapping.h:16:0,
                 from /tmp/ndn-lite/tests/ndn-lite/app-support/security-bootstrapping.c:11:
/tmp/ndn-lite/tests/ndn-lite/app-support/../forwarder/forwarder.h:167:1: note: expected ‘void *’ but argument is of type ‘const uint8_t * const {aka const unsigned char * const}’
 ndn_forwarder_express_interest(uint8_t* interest, size_t length,
 ^

yan-foto · 2020-09-03T21:40:47Z

My time is really tight here. I'd appreciate it if the last mile can be taken care of by the core team.

@yoursunny this is exactly what I'm talking about 👆

yoursunny · 2020-09-03T21:50:29Z

I'd appreciate it if the last mile can be taken care of by the core team.

I think you are a better developer than most others, and you deserve a spot on the core team or become the project manager.

tianyuan129 · 2020-09-03T21:50:59Z

I can change this quickly, but could you take a look at the rest?

sure, can I merge it first your project can move on?

yoursunny · 2020-09-03T21:51:59Z

sure, can I merge it first your project can move on?

I'd recommend not to merge the current code, since it leaves the codebase in an uncompilable state.
All warnings have to be resolved.

yan-foto · 2020-09-03T21:54:38Z

I think you are a better developer than most others, and you deserve a spot on the core team or become the project manager.

you should've been a politician or at least be awarded with a medal or something for your convincing skills :)

sure, can I merge it first your project can move on?

Thanks for the offer, but let's wait until everything is resolved. @yoursunny somehow convinced me to do take care of the rest...

Changes fix following errors: - ndn-lite/app-support/security-bootstrapping.c:286:46: error: passing argument 5 of ‘ndn_forwarder_express_interest’ discards ‘const’ qualifier from pointer target type NOTE: This specific error was caused by inconsistend programming in declaring a pointer as const in one place but assigning it to other pointers which are not const.

Changes fix following warnings: - adaptation/udp/udp-face.c:206:7: error: variable ‘ret’ set but not used NOTE: it seems that original authors abruptly stopped coding...

Changes fix follwoing warnings: - tests/adaptation/security/ndn-lite-rng-posix-crypto-impl.c:31:18: error: implicit declaration of function ‘read’; did you mean ‘creat’?

yan-foto · 2020-09-04T22:02:52Z

So the warnings are fixed and the tests all pass. During this process I also discovered some bugs which makes me really wonder why these were not flagged by the unit tests 🤔

Anyhow, it has been a while since I've used Travis CI on GitHub and according to [its docs] you should first enable it for GitHub before using it. If I understand it correctly, it doesn't pick up jobs automatically anymore as it used to do. Nonetheless I added a preliminary CI script, wondering if it's gonna work!

yoursunny · 2020-09-04T22:09:51Z

you should first enable it for GitHub before using it

For Travis CI, you need to enable it on your fork first.
Independently, a maintainer needs to enable it for this repository.

yoursunny

I can confirm there's no more warning on Ubuntu 16.04 with GCC.
Travis script is the only problem. @yan-foto needs to enable Travis on his fork to confirm the result.

.travis.yml

yan-foto · 2020-09-05T10:05:16Z

OK folks, everything works as expected. I really didn't want to work on shabbat, but I guess I also couldn't leave this cake half-baked!

The unit tests pass but I still think the code needs more rigorous testing. So maybe we first merge this on the develop branch and have it tested by some of our best (students). What do you reckon?

yoursunny · 2020-09-05T10:27:50Z

@tianyuan129 needs to install Travis CI in this repository.

tianyuan129 · 2020-09-05T10:51:02Z

Sure. Currently I'm not the admin for this repository. Let me ask for the permission first and then I'll install it.

yoursunny · 2020-09-05T14:14:35Z

@Zhiyi-Zhang needs to install Travis CI in this repository.

tianyuan129 · 2020-09-06T03:20:27Z

just installed Travis CI on develop branch: d60a114.

Hanwen Zhang and others added 21 commits January 10, 2020 15:44

Initial commit

bc9408e

Add Getting Started to README

5f5605d

Add aes, data, encoder/decoder, hmac, sign-verify unit tests

f62b49f

Update README

d1a64c1

Add the rest of rewritten unit tests

9106cc7

Update main.c

ba7a2df

Increase coverage on forwarder test

e88a2ec

Add more tests on fragmentation support, verify assembly result

502de3f

Add FIB tests; Add forwarder with fragmentation support test

1b11fed

Fix comments

b1b2f99

Update .gitmodules

24f033d

build cunit from source

0a9954d

Fix compiler warnings: 'implicit-fallthrough'

d1ef47b

Changes fix following error: - encode/wrapper-api.c:405:12: warning: this statement may fall through [-Wimplicit-fallthrough=]

Fix compiler warnings: 'type-limit'

dfd92ba

Changes fix following errors: - security/ndn-lite-hmac.c:146:18: warning: comparison of unsigned expression < 0 is always false (3 errors on the same line)

Fix compiler warnings: 'old-style-definition'.

bc79fcc

Changes fix following errors: - encode/key-storage.c:126:1: error: old-style function definition - encode/key-storage.c:156:1: error: old-style function definition

Fix compiler warnings: 'no-return-type'

bafbbd4

Changes fixes following warnings: - forwarder/forwarder.c:314:1: error: control reaches end of non-void function

Fix compiler warnings: 'unused-but-set-variable'

1a224c2

Changes fix following warnings: - forwarder/name-tree.c:83:17: error: variable ‘last_node’ set but not used NOTE: this unused variable seems to be remainder of a 'copy/paste' action!

yan-foto force-pushed the feature/warnings-fix branch from 4847b10 to ae85f84 Compare August 27, 2020 16:48

yan-foto added 2 commits September 1, 2020 22:15

Add 'tests/' from commit '0a9954d0e414e90b99ee19ddb196962b123ca636'

967d79c

git-subtree-dir: tests git-subtree-mainline: ae85f84 git-subtree-split: 0a9954d

yan-foto mentioned this pull request Sep 1, 2020

Compiler warnings (sign-compare, type-limites, etc.) #74

Closed

Fix compiler warnings: 'format'

ffdea3f

Changes here make format strings portable and cross platform compatible.

Fix compiler warning: 'return-type'

1653f8c

yoursunny suggested changes Sep 3, 2020

View reviewed changes

yan-foto added 5 commits September 4, 2020 23:04

Fix compiler warning: 'unused-but-set-variable'

f0e56de

Changes fix following warnings: - adaptation/udp/udp-face.c:206:7: error: variable ‘ret’ set but not used NOTE: it seems that original authors abruptly stopped coding...

Fix compiler warning: 'unused variable'.

7645761

Fix compiler warning: 'implicit-function-declaration'

8ec194e

Changes fix follwoing warnings: - tests/adaptation/security/ndn-lite-rng-posix-crypto-impl.c:31:18: error: implicit declaration of function ‘read’; did you mean ‘creat’?

Fix minor bug in cmake (target specific flag was applied to others)

e25075d

yoursunny suggested changes Sep 4, 2020

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

.travis.yml Show resolved Hide resolved

yoursunny reviewed Sep 4, 2020

View reviewed changes

.travis.yml Show resolved Hide resolved

yan-foto force-pushed the feature/warnings-fix branch 2 times, most recently from 739f305 to 383db5a Compare September 5, 2020 09:50

Preliminary Travis CI config (let's hope it works!).

3bb4bdc

yan-foto force-pushed the feature/warnings-fix branch from 383db5a to 3bb4bdc Compare September 5, 2020 09:56

yoursunny approved these changes Sep 5, 2020

View reviewed changes

tianyuan129 changed the base branch from master to develop September 5, 2020 10:21

tianyuan129 merged commit a44bda3 into named-data-iot:develop Sep 5, 2020

yoursunny mentioned this pull request Sep 18, 2020

Archive this repository named-data-iot/ndn-lite-tests#1

Open

yan-foto mentioned this pull request Sep 30, 2020

Question: code styling/formatting guidelines #83

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compiler warnings #75

Fix compiler warnings #75

yan-foto commented Aug 27, 2020

yan-foto commented Aug 27, 2020

yoursunny commented Aug 27, 2020

yoursunny commented Sep 1, 2020 •

edited

Loading

yan-foto commented Sep 1, 2020 via email •

edited

Loading

yoursunny left a comment

yan-foto commented Sep 3, 2020

yoursunny commented Sep 3, 2020

tianyuan129 commented Sep 3, 2020

yoursunny commented Sep 3, 2020

yan-foto commented Sep 3, 2020 •

edited

Loading

yan-foto commented Sep 4, 2020

yoursunny commented Sep 4, 2020

yoursunny left a comment

yan-foto commented Sep 5, 2020

yoursunny commented Sep 5, 2020

tianyuan129 commented Sep 5, 2020

yoursunny commented Sep 5, 2020

tianyuan129 commented Sep 6, 2020

Fix compiler warnings #75

Fix compiler warnings #75

Conversation

yan-foto commented Aug 27, 2020

yan-foto commented Aug 27, 2020

yoursunny commented Aug 27, 2020

yoursunny commented Sep 1, 2020 • edited Loading

yan-foto commented Sep 1, 2020 via email • edited Loading

yoursunny left a comment

Choose a reason for hiding this comment

yan-foto commented Sep 3, 2020

yoursunny commented Sep 3, 2020

tianyuan129 commented Sep 3, 2020

yoursunny commented Sep 3, 2020

yan-foto commented Sep 3, 2020 • edited Loading

yan-foto commented Sep 4, 2020

yoursunny commented Sep 4, 2020

yoursunny left a comment

Choose a reason for hiding this comment

yan-foto commented Sep 5, 2020

yoursunny commented Sep 5, 2020

tianyuan129 commented Sep 5, 2020

yoursunny commented Sep 5, 2020

tianyuan129 commented Sep 6, 2020

yoursunny commented Sep 1, 2020 •

edited

Loading

yan-foto commented Sep 1, 2020 via email •

edited

Loading

yan-foto commented Sep 3, 2020 •

edited

Loading