Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the compile error once enabled Werror #11325

Closed
wants to merge 1 commit into from

Conversation

xkqian
Copy link
Contributor

@xkqian xkqian commented Mar 13, 2020

On 32 bit operating system, execute the following command "linux32 ./config -Werror -g3 --prefix=xxx; make", it reported error "type mismatch in the file test/cmp_ctx_test.c: TEST_note("total=%d len=%ld msg='%s'\n", msg_total_size, strlen(msg), msg);".
On 32 bit system, size_t is defined as unsigned int, this is the return type of strlen(), but the code use %ld to printf it, when compiling, type mismatch warning will be reported. For printf, %zu should be the right specifier of size_t, change to avoid the warning.

On 32 bit operating system,size_t is defined as unsigned int,
this is the return type of strlen(), but it isn't aligned with the %ld,
when compiling, warning will be reported.
Change the type to %zu to avoid the warning.

Change-Id: I2943d0dfba88ef42892f14230242008473d6263b
@kroeckx
Copy link
Member

kroeckx commented Mar 13, 2020

We target C89/C90. As far as I know, C90 doesn't know about the z modifier, and so we can't use it. This would mean that all other code making use of TEST_note() with %z needs to be modified.

Note that our BIO_*printf() functions can use it, it has it's own implementation, but test_note() ends up calling vfprintf().

@xkqian
Copy link
Contributor Author

xkqian commented Mar 13, 2020

We target C89/C90. As far as I know, C90 doesn't know about the z modifier, and so we can't use it. This would mean that all other code making use of TEST_note() with %z needs to be modified.

Note that our BIO_*printf() functions can use it, it has it's own implementation, but test_note() ends up calling vfprintf().

Actually, we can find many places use %z in OpenSSL for TEST_note(). Example as below:

./test/poly1305_internal_test.c:1561: TEST_info("Poly1305 test #%d/%zu+%zu failed.",
./test/sparse_array_test.c:52: TEST_note("iteration %zu", i + 1);
./test/sparse_array_test.c:57: TEST_note("iteration %zu / %zu", i + 1, j + 1);
./test/sparse_array_test.c:164: TEST_note("failed at iteration %zu", i + 1);
./test/drbgtest.c:1377: TEST_note("DRBG %zd case %zd block %zd", n / crngt_num_cases,
./test/testutil/tests.c:238:DEFINE_COMPARISONS(size_t, size_t, "%zu")
./test/ssl_cert_table_internal_test.c:32: TEST_error("Invalid table entry for certificate type %s, index %zu",
./test/bioprinttest.c:117: { SIZE_MAX, "%zu", (sizeof(size_t) == 4 ? "4294967295"
./test/bioprinttest.c:124: { SIZE_MAX / 2 + 1, "%zi", (sizeof(size_t) == 4 ? "-2147483648"
./test/bioprinttest.c:127: { 0, "%zu", "0" },
./test/bioprinttest.c:128: { 0, "%zi", "0" },
./test/params_api_test.c:471: TEST_note("iteration %zu var %s", j + 1, int_names[j]);
./test/params_api_test.c:482: TEST_note("iteration %zu var %s", j + 1, uint_names[j]);
./test/siphash_internal_test.c:184: TEST_info("size %zu vs %d and %d", expectedlen,
./test/siphash_internal_test.c:248: TEST_info("SipHash test #%d/%zu+%zu failed.",
./test/dtls_mtu_test.c:107: TEST_info("record %zu for payload %zu", reclen, s);

Fristly we try to use type cast, but to align with the OpenSSL exist style, we change it like this. As we know, %zu is one c99 addition, can anyone help answer whether we are targeting c89/c90, and whether the exist %zu is proper?

@xkqian
Copy link
Contributor Author

xkqian commented Mar 13, 2020

We target C89/C90. As far as I know, C90 doesn't know about the z modifier, and so we can't use it. This would mean that all other code making use of TEST_note() with %z needs to be modified.

Note that our BIO_*printf() functions can use it, it has it's own implementation, but test_note()
ends up calling vfprintf().


Sorry, do you mean we should discard all of the z% in the OpenSSL code except in the functions BIO_*printf()?
As we know, %z is one good specifier for size_t, but only be available after c99, it can dynamically detect the bits of the operating system, once we change it like %lu, or ld%, maybe force type cast needed, this will make the code look ugly, do you have any good suggestions about how to change them? I can see many places use the %z.

@mattcaswell
Copy link
Member

Note that our BIO_*printf() functions can use it, it has it's own implementation, but test_note() ends up calling vfprintf().

As far as I can see TEST_note is a macro for test_note, which calls test_vprintf_stderr which calls BIO_vprintf. The only place where this is not the case is in bioprinttest.c which has its own custom implementation of test_vprintf_stderr that does call vfprintf directly.

So, I think actually we are ok.

@mattcaswell mattcaswell added the approval: review pending This pull request needs review by a committer label Mar 13, 2020
@t8m t8m added approval: done This pull request has the required number of approvals branch: master Merge to master branch and removed approval: review pending This pull request needs review by a committer labels Mar 13, 2020
@kroeckx
Copy link
Member

kroeckx commented Mar 13, 2020 via email

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@xkqian
Copy link
Contributor Author

xkqian commented Mar 16, 2020

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.


Hi mattcaswell&t8m, What further actions should I do? @mattcaswell @t8m

@t8m t8m added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Mar 16, 2020
openssl-machine pushed a commit that referenced this pull request Mar 16, 2020
On 32 bit operating system,size_t is defined as unsigned int,
this is the return type of strlen(), but it isn't aligned with the %ld,
when compiling, warning will be reported.
Change the type to %zu to avoid the warning.

Change-Id: I2943d0dfba88ef42892f14230242008473d6263b

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #11325)
@t8m
Copy link
Member

t8m commented Mar 16, 2020

Merged to master. Thank you.

@t8m t8m closed this Mar 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants