consider clang -std=gnu99 in Travis for better C99 portability

In #32495 (moved), we discovered that -Wtypedef-redefinition doesn't seem to work on clang unless -std=gnu99 or similar is used. (typedef redefinitions are apparently allowed in C11) It does seem to work on FreeBSD's cc for some reason, but we don't use FreeBSD in Travis.

We should consider using something like -std=gnu99 in our Travis configuration for clang, so we can catch additional instances of this sort of portability problem earlier.

changed milestone to %Tor: 0.2.9.x-final

added 029-backport 035-backport 040-backport 041-backport 042-backport actualpoints::0.5 component::core tor/tor milestone::Tor: 0.2.9.x-final owner::teor points::0.1 portability priority::medium resolution::fixed reviewer::catalyst severity::normal sponsor::31-can status::closed tor-ci type::enhancement labels

See my PR, based on #32495 (moved):

• master: https://github.com/torproject/tor/pull/1538

I'm now seeing how far I can backport this change.

Trac:
Keywords: N/A deleted, 041-backport, 042-backport, 040-backport, 029-backport, 035-backport added
Type: defect to enhancement
Owner: N/A to teor
Points: N/A to 0.1
Status: new to assigned
Milestone: Tor: unspecified to Tor: 0.4.3.x-final
Sponsor: N/A to Sponsor31-can

It looks like 0.2.9 works.

See my PRs:

• 0.2.9: https://github.com/torproject/tor/pull/1539
• 0.3.5: https://github.com/torproject/tor/pull/1540
• 0.4.2: https://github.com/torproject/tor/pull/1541

I also tested that it does fail without #32495 (moved): https://travis-ci.org/teor2345/tor/builds/612156095 (It only fails on clang.)

Trac:
Actualpoints: N/A to 0.2
Status: assigned to needs_review
Reviewer: N/A to catalyst

Replying to teor:

It looks like 0.2.9 works.

See my PRs:

• 0.2.9: https://github.com/torproject/tor/pull/1539
• 0.3.5: https://github.com/torproject/tor/pull/1540
• 0.4.2: https://github.com/torproject/tor/pull/1541

I also tested that it does fail without #32495 (moved): https://travis-ci.org/teor2345/tor/builds/612156095 (It only fails on clang.) Thanks! These look good.

Trac:
Status: needs_review to merge_ready

Are we doing this with all clang builds, or only some clang builds? I vote for "only some", since we may build differently (eg with different autoconf results) if we do not have std=gnu99 set.

Replying to nickm:

Are we doing this with all clang builds, or only some clang builds? I vote for "only some", since we may build differently (eg with different autoconf results) if we do not have std=gnu99 set.

catalyst had the same question in #tor-dev, so I've changed Travis to run using a macOS clang -std=gnu99, and a Linux gcc -std=gnu99. (Just in case gcc has extra warnings, too.)

There are fewer conflicts, so we just need to merge:

• 0.2.9: ​https://github.com/torproject/tor/pull/1539
• 0.3.5: ​https://github.com/torproject/tor/pull/1540

Test branches are here:

• https://travis-ci.org/teor2345/tor/branches

Trac:
Status: merge_ready to needs_review
Actualpoints: 0.2 to 0.3

Replying to teor:

Replying to nickm:

Are we doing this with all clang builds, or only some clang builds? I vote for "only some", since we may build differently (eg with different autoconf results) if we do not have std=gnu99 set.

catalyst had the same question in #tor-dev, so I've changed Travis to run using a macOS clang -std=gnu99, and a Linux gcc -std=gnu99. (Just in case gcc has extra warnings, too.)

There are fewer conflicts, so we just need to merge:

• 0.2.9: ​https://github.com/torproject/tor/pull/1539
• 0.3.5: ​https://github.com/torproject/tor/pull/1540

Test branches are here:

• https://travis-ci.org/teor2345/tor/branches Thanks! Mostly looks good. Your ticket32500_master branch seems to be failing Travis. It looks like a coccinelle problem, so maybe it's unrelated? Maybe you could try rebasing it to see if it was a transient coccinelle problem on master?

Trac:
Status: needs_review to needs_information

Replying to catalyst:

Thanks! Mostly looks good. Your ticket32500_master branch seems to be failing Travis. It looks like a coccinelle problem, so maybe it's unrelated? Maybe you could try rebasing it to see if it was a transient coccinelle problem on master?

That's this test, which is meant to fail:

I also tested that it does fail without #32495 (moved): https://travis-ci.org/teor2345/tor/builds/612156095

Trac:
Status: needs_information to needs_review

Oh, I see now. Same branch name, different build, different failure.

I merged the current master into ticket32500_master, and re-pushed. All the local checks pass, and all the master checks pass, so let's see how it goes.

https://travis-ci.org/teor2345/tor/builds/617113924

There's definitely something weird going on here.

The -std=gnu99 flag shouldn't affect coccinelle, but maybe it's reading the contents of $CC, and interpreting them differently? It's also weird that the gcc/trusty build fails, but the clang/macOS build passes.

Replying to teor:

There's definitely something weird going on here.

The -std=gnu99 flag shouldn't affect coccinelle, but maybe it's reading the contents of $CC, and interpreting them differently? It's also weird that the gcc/trusty build fails, but the clang/macOS build passes. Yeah those inconsistencies bother me as well.

Also I think coccinelle will see CC=gcc in its environment. (We pass CC="$CC -std=gnu99" to ./configure, but that shouldn't change the CC environment variable.

For what it's worth, I'm not able to reproduce this failure on Xenial.

Further investigation reveals that check-cocci probably hasn't ever worked on Trusty because of an older version of coccinelle that has a harder time parsing our files.

It looks like the setting of env: C_DIALECT_OPTIONS="-std=gnu99" in .travis.yml overrode the the existing env: CHUTNEY="yes" CHUTNEY_ALLOW_FAILURES="2" SKIP_MAKE_CHECK="yes" in that matrix entry, which likely caused make check-cocci to run for the first time on Trusty, unmasking a latent failure. (See https://travis-ci.org/tlyu/tor/builds/617437732 for a diagnostic.) The other Linux matrix entries run on Bionic.

So we should probably attach the gcc -std=gnu99 variant to a different matrix entry.

Trac:
Status: needs_review to needs_revision

Thanks for helping diagnose that. I merged the two env lists, and re-merged all the branches.

(I couldn't find another matrix entry, most of the rest of them disable large amounts of code.)

Here are the PRs for review:

• 0.2.9: ​​https://github.com/torproject/tor/pull/1539
• 0.3.5: ​​https://github.com/torproject/tor/pull/1540

Test branches are here:

• ​https://travis-ci.org/teor2345/tor/branches

Replying to teor:

Thanks for helping diagnose that. I merged the two env lists, and re-merged all the branches.

(I couldn't find another matrix entry, most of the rest of them disable large amounts of code.)

Here are the PRs for review:

• 0.2.9: ​​https://github.com/torproject/tor/pull/1539
• 0.3.5: ​​https://github.com/torproject/tor/pull/1540

Test branches are here:

• ​https://travis-ci.org/teor2345/tor/branches Thanks! This looks good now. The main drawback I can think of using for the chutney matrix entry is that we'll be using the gcc in Trusty, rather than something newer. Maybe it'll be OK.

Trac:
Status: needs_revision to merge_ready

We were using the trusty gcc for all our builds until about a month ago :-)

I'm going to experiment with using the NSS gcc in later Tor versions. There are two advantages:

• we'll test the NSS-specific code with -std=gnu99
• we'll get a later gcc version

Same PRs and tests as before, but it only affects 0.3.5 and later.

(Thanks for your patience with this ticket!)

Trac:
Status: merge_ready to needs_review

Trac:
Actualpoints: 0.3 to 0.5

Replying to teor:

We were using the trusty gcc for all our builds until about a month ago :-)

I'm going to experiment with using the NSS gcc in later Tor versions. There are two advantages:

• we'll test the NSS-specific code with -std=gnu99
• we'll get a later gcc version

Same PRs and tests as before, but it only affects 0.3.5 and later.

(Thanks for your patience with this ticket!) Thanks! Looks good now.

I do think we want to make a comment explaining that SKIP_MAKE_CHECK="yes" is required on Trusty at the moment because its coccinelle is too old to parse our C files. Would you rather to it as part of this ticket, or as a new ticket?

Trac:
Status: needs_review to merge_ready

If we do it this way, can we please have another ticket to make Trusty skip less? Maybe we should have a "SKIP_COCCI" that it can use instead, and have our makefile acknowledge it.

(This can be a separate ticket.)

Replying to catalyst:

Further investigation reveals that check-cocci probably hasn't ever worked on Trusty because of an older version of coccinelle that has a harder time parsing our files.

make check-cocci is quite new, we upgraded to bionic so we could have a new enough version of coccinelle: https://trac.torproject.org/projects/tor/ticket/31919#comment:3

I did some extra tests, the required coccinelle version is some version in this range:

• 1.0.0-rc21

• <= 1.0.4

I couldn't find any versions lower than 1.0.4 in debian, ubuntu, or macOS homebrew, so I suggest we require 1.0.4.

Replying to catalyst:

I do think we want to make a comment explaining that SKIP_MAKE_CHECK="yes" is required on Trusty at the moment because its coccinelle is too old to parse our C files. Would you rather to it as part of this ticket, or as a new ticket?

Replying to nickm:

If we do it this way, can we please have another ticket to make Trusty skip less? Maybe we should have a "SKIP_COCCI" that it can use instead, and have our makefile acknowledge it.

We only have one Trusty CI job right now, which runs chutney without make check. We'd have the chutney job on bionic, but that's blocked on #32240 (moved).

But you're right, our make check should run without failing, even if people have a really old coccinelle.

I suggest we require coccinelle version 1.0.4 in check_cocci_parse.sh. That way, our changes work in the Makefile and the git hooks. And we can put the relevant comment/notice there.

We should also check what happens if we install coccinelle on Windows.

I opened #32663 (moved) for this fix.

Merged to master.

I squashed the 0.2.9 branch before merging, because there were two commits with very similar descriptions, that should have been one amended commit. Then I re-did the merge to 0.3.5.

Trac:
Resolution: N/A to fixed
Status: merge_ready to closed

Trac:
Milestone: Tor: 0.4.3.x-final to Tor: 0.2.9.x-final

closed

changed time estimate to 48m

added 4h of time spent

moved to tpo/core/tor#32500 (closed)