Opened 4 weeks ago

Closed 8 days ago

Last modified 40 hours ago

#32500 closed enhancement (fixed)

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

Reported by: catalyst Owned by: teor
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: portability, tor-ci, 029-backport, 035-backport, 040-backport, 041-backport, 042-backport
Cc: Actual Points: 0.5
Parent ID: Points: 0.1
Reviewer: catalyst Sponsor: Sponsor31-can

Description

In #32495, 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.

Child Tickets

Change History (20)

comment:1 Changed 4 weeks ago by teor

Keywords: 029-backport 035-backport 040-backport 041-backport 042-backport added
Milestone: Tor: unspecifiedTor: 0.4.3.x-final
Owner: set to teor
Points: 0.1
Sponsor: Sponsor31-can
Status: newassigned
Type: defectenhancement

See my PR, based on #32495:

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

comment:2 Changed 4 weeks ago by teor

Actual Points: 0.2
Reviewer: catalyst
Status: assignedneeds_review

It looks like 0.2.9 works.

See my PRs:

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

comment:3 in reply to:  2 Changed 4 weeks ago by catalyst

Status: needs_reviewmerge_ready

Replying to teor:

It looks like 0.2.9 works.

See my PRs:

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

Thanks! These look good.

comment:4 Changed 4 weeks ago by 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.

comment:5 in reply to:  4 ; Changed 4 weeks ago by teor

Actual Points: 0.20.3
Status: merge_readyneeds_review

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:

Test branches are here:

comment:6 in reply to:  5 ; Changed 2 weeks ago by catalyst

Status: needs_reviewneeds_information

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:

Test branches are here:

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?

comment:7 in reply to:  6 Changed 2 weeks ago by teor

Status: needs_informationneeds_review

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: https://travis-ci.org/teor2345/tor/builds/612156095

Last edited 2 weeks ago by teor (previous) (diff)

comment:8 Changed 2 weeks ago by teor

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

comment:9 Changed 2 weeks ago by 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.

comment:10 in reply to:  9 Changed 2 weeks ago by catalyst

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.

comment:11 Changed 2 weeks ago by catalyst

Status: needs_reviewneeds_revision

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.

comment:12 Changed 2 weeks ago by 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:

Test branches are here:

comment:13 in reply to:  12 Changed 2 weeks ago by catalyst

Status: needs_revisionmerge_ready

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:

Test branches are here:

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.

comment:14 Changed 13 days ago by teor

Status: merge_readyneeds_review

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!)

comment:15 Changed 13 days ago by teor

Actual Points: 0.30.5

comment:16 in reply to:  14 Changed 8 days ago by catalyst

Status: needs_reviewmerge_ready

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?

comment:17 Changed 8 days ago by 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.

(This can be a separate ticket.)

comment:18 in reply to:  17 Changed 8 days ago by teor

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.

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 for this fix.

comment:19 Changed 8 days ago by teor

Resolution: fixed
Status: merge_readyclosed

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.

comment:20 Changed 40 hours ago by teor

Milestone: Tor: 0.4.3.x-finalTor: 0.2.9.x-final
Note: See TracTickets for help on using tickets.