patch meas_astrom v11.0 to prevent a.d.n SIGSEVs #22
Conversation
|
If this patch is by @mjuric how come the commit is not by him? Use |
|
I lifted the patch out of an unrelated repo, and it is part of a large changeset. It wouldn't have been helpful to export the entire changeset as a diff or play changes with sub-tree in order to cherry-pick it. If you want, I can fudge the commit author... |
|
I would prefer author attribution was done properly even for small manually applied patches. |
jhoblitt
force-pushed
the
tickets/DM-4983-assertion-fix
branch
from
46aa9b9
to
1e779a4
Compare
|
FWIW, here's the original patch (and a description of what's going on -- lessons learned all around!): mjuric/conda-lsst@c38cc6b |
|
@mjuric Thanks -- I never would have found that due to a rebase. I will fix up this PR in a few mins. |
|
-O3 #defining NDEBUG is new. And bad. |
jhoblitt
force-pushed
the
tickets/DM-4983-assertion-fix
branch
from
1e779a4
to
2433733
Compare
jhoblitt
changed the title
|
@RobertLuptonTheGood Yup. Bad @dstndstn :). Re-enabling asserts may be a good follow-up to this patch... Offending line: https://github.com/dstndstn/astrometry.net/blob/master/util/makefile.common#L251 |
|
You don't have debug builds? Tsk thyself |
|
And the underlying bug was where? I didn't see any patches flowing upstream to astrometry.net |
|
With C++ building -O0 is something like 10x slower and optimising shouldn't generate bugs. Traditionally building NDEBUG and non-debugged are orthogonal issues (and the former is only done after profiling). |
Fair point. I never figured out what the underlying bug was :(. I did e-mail some Dustin guy to check that it was a bug, though ;-). |
W/o this patch, astrometry.net will SIGSEV under some circumstances (*) when running the SDSS demo. By default a.d.n is built with optimizations turned on, which (among other things) disables checking of assert statements. That would've caught the fact we were calling healpixDistance() with healpix=-1, which is illegal. This patch changes the test in isWithinRange() to test for healpix == -1 (instead of nside == 0) when deciding whether it's permissible to call healpixDistance(). (*) This bug was discovered when the stack was built on CentOS 5 and the generated binaries were executed on CentOS 6 (with different glibc). Running CentOS5-built binaries on CentOS 5 (and CentOS 6 binaries on CentOS 6) did not reveal it. Demonstrates how testing in different environments can reveal subtle bugs (**). (**) This bug would've been caught immediately if assertions weren't turned off in astrometry.net. We should turn them back on for our builds.
jhoblitt
force-pushed
the
tickets/DM-4983-assertion-fix
branch
from
2433733
to
56eee04
Compare
patch meas_astrom v11.0 to prevent a.d.n SIGSEVs
fixed upstream: lsst/meas_astrom#22
No description provided.