Opened 4 years ago

Closed 4 years ago

#18242 closed defect (fixed)

Revert no-assertions-on-coverage, or make it controlled by an option.

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version:
Severity: Major Keywords: TorCoreTeam201601
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In 1228dd293b60a we made TOR_COVERAGE equavalent to NDEBUG so that branch coverage could be accurate.

But we hate NDEBUG.

I just spent 20 minutes too long debugging a problem because I'd forgotten that I was building with enable_coverage and that as such I shouldn't expect tor_assert to do anything.

We should either revert 1228dd293b60a, or make an --enable-branch-coverage option for configure that you have to use if you want to do branch coverage this way.

Child Tickets

Change History (8)

comment:1 Changed 4 years ago by nickm

Owner: set to nickm
Status: newaccepted

comment:2 Changed 4 years ago by nickm

(This isn't a super-high severity thing, since it only happens in testing builds of our files, never in the "tor" binary that people use.)

comment:3 Changed 4 years ago by Sebastian

Hrm, ok. I still think that behaviour is kind of sensible, but it sucks that we can't ensure that assertions don't have side-effects, so I see the debugging issue. I guess using --enable-coverage is way more common during regular development than I thought.

comment:4 Changed 4 years ago by nickm

Status: acceptedneeds_review

bug18242 in my public repo does the obvious thing, with a configure option --disable-asserts-in-tests. It is not tested.

comment:5 Changed 4 years ago by Sebastian

The patch doesn't work. Investigating.

comment:6 Changed 4 years ago by Sebastian

Ok, this needs a bit more work than I thought. I'll provide a followup patch in a bit

comment:7 Changed 4 years ago by Sebastian

bug18242 in my repo has an suggested fix. It makes sure you can't disable-asserts-in-tests unless you're doing a coverage build as well as fixing the commit

comment:8 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Thanks, merging!

Note: See TracTickets for help on using tickets.