Thanks peter for the details review. I've asked upstream to merge hunk #2 (invalid code to error change) in any case.
At the moment they aren't interested in the rest of the patch since it fixes a problem on an old hardware.I wouldn't ask them to reconsider without a good reason. If even Debian doesn't use this hardware, I don't see the point of trying to push this. In any case, the removal of the patch will be documented for future reference if needed. Waiting for Martin's feedback before doing the change in Debian. Kaplan On Fri, Oct 4, 2013 at 8:12 PM, peter green <plugw...@p10link.net> wrote: > > I think the patch does make sense, but only for doing floating point >> arithmetic in PHP on very old hardware. >> >> Lets take a look, there are three hunks in the patch. > > The first hunk is clearly altering a block of logic that determines > floating point format details of the system that the code is being built > for. The third hunk is responding to new defintions from the first hunk. > The second hunk is an improvement in error reporting (replacing invalid > code with a proper #error) > > The first step in determining what is going on is to go through the > defines that are looked at by the code (both the old code and the new code) > and determine their meanings (a lot of information taken from > https://wiki.debian.org/**ArmEabiPort<https://wiki.debian.org/ArmEabiPort>). > > __arm__ is defined by both armel and armhf > __thumb__ is defined when building for thunb (in debian armhf does this by > default armel doesn't but command line switches can be used to turn thumb > on or off in both cases) > __VFP_FP__ is defined by default on both armel and armhf. However from > stuff I have read on the wiki it appears that it is not defined when > building for armel with a maverick FPU. Afaict building for armel with a > maverick FPU requires a patched compiler. > __MAVERICK__ is not defined by default on either debian armel or debian > armhf but it is apparently defined when building armel packages for a > maverick FPU. From a floating point POV it seems to mean much the same as > __VFP_FP__, that is that "natrually" ordered floating point numbers are in > use. > __ARMEL__ is apparently defined on all little endian arm systems including > old abi ones which use the mixed endian floats > > Tracing through the hunks with this information we see that the version of > the hunk in the patch makes a number of changes to the logic. > > 1: Without the patch __MAVERICK__ is ignored which would result in php > incorrectly assuming that the system was using mixed endian floats when > building armel packges for the maverick FPU. > 2: Without the patch the system basically assumes that big endian arm > systems don't exist. With the patch it appears to handle them in a sane way. > 3: It treats __thumb__ as equivilent to __arm__, I presume this is a > response to some older compiler that didn't define __arm__ when building > thumb code. > > None of these fixes are required for the settings debian currently build > under. Having said that just because debian doesn't currently build with > those settings does not mean that people may not want to build with them in > the future. Removing logic to correctly handle build configurations such > that people who want to build for those configurations have to rediscover > why things are misbehaving in strange ways does not seem like a nice thing > to do to me. IMO this patch should have been upstreamed ages ago. > > I'm ccing martin guy because he still appears to be involved with debian > stuff for maverick, at least he is still editing the wiki page relating to > it. >
