You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Sander Temme <sa...@temme.net> on 2010/11/04 19:46:42 UTC
[PATCH] %lld support in apr_snprintf()
Folks,
I was seeing test failures on Darwin in both the APR testsuite and httpd perl-framework. The %lld sprintf format character was incorrectly parsed, and "%ld" written instead of the substituted value.
This small patch against APR trunk fixes that:
Index: strings/apr_snprintf.c
===================================================================
--- strings/apr_snprintf.c (revision 1031121)
+++ strings/apr_snprintf.c (working copy)
@@ -832,6 +832,11 @@
else if (*fmt == 'l') {
var_type = IS_LONG;
fmt++;
+ /* Catch the %lld type modifier for long long and its ilk */
+ if (*fmt == 'l') {
+ var_type = IS_QUAD;
+ fmt++;
+ }
}
else if (*fmt == 'h') {
var_type = IS_SHORT;
Thanks,
S.
--
sander@temme.net http://www.temme.net/sander/
PGP FP: FC5A 6FC6 2E25 2DFD 8007 EE23 9BB8 63B0 F51B B88A
View my availability: http://tungle.me/sctemme
Re: [PATCH] %lld support in apr_snprintf()
Posted by Chris Knight <Ch...@nasa.gov>.
Reported many times (by me and others) and many patches suggested. But apparently no progress on addressing this issue. :^(
https://issues.apache.org/bugzilla/show_bug.cgi?id=48476
On Nov 4, 2010, at 11:46 AM, Sander Temme wrote:
> Folks,
>
> I was seeing test failures on Darwin in both the APR testsuite and httpd perl-framework. The %lld sprintf format character was incorrectly parsed, and "%ld" written instead of the substituted value.
>
> This small patch against APR trunk fixes that:
>
> Index: strings/apr_snprintf.c
> ===================================================================
> --- strings/apr_snprintf.c (revision 1031121)
> +++ strings/apr_snprintf.c (working copy)
> @@ -832,6 +832,11 @@
> else if (*fmt == 'l') {
> var_type = IS_LONG;
> fmt++;
> + /* Catch the %lld type modifier for long long and its ilk */
> + if (*fmt == 'l') {
> + var_type = IS_QUAD;
> + fmt++;
> + }
> }
> else if (*fmt == 'h') {
> var_type = IS_SHORT;
>
> Thanks,
>
> S.
>
> --
> sander@temme.net http://www.temme.net/sander/
> PGP FP: FC5A 6FC6 2E25 2DFD 8007 EE23 9BB8 63B0 F51B B88A
>
> View my availability: http://tungle.me/sctemme
> <apr-snprintf.patch>
Re: [PATCH] %lld support in apr_snprintf()
Posted by Graham Leggett <mi...@sharp.fm>.
On 08 Nov 2010, at 5:27 PM, Jim Jagielski wrote:
> If one forces *just* 64bit, then, afaict, the patch is not needed.
> It's only if one builds APR with both i386 and x86_64 that
> things break...
Building with both is default behaviour, and has been default
behaviour since universal binaries were introduced into MacOSX in 2005.
Regards,
Graham
--
Re: [PATCH] Re: [PATCH] %lld support in apr_snprintf()
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
On 12/17/2010 6:31 AM, Jim Jagielski wrote:
>
> Oh, yeah. Well, it's not only the format but
> everything as well... After all, if long long is 128bits,
> you don't want to use apr_strtoi64 either.
Exactly
> I would suggest that we tackle that issue separately?
Sure, of course. Show me broken code, and I'll point out where it's
broke, but as the patch rearranges things, rearrange away.
Re: [PATCH] Re: [PATCH] %lld support in apr_snprintf()
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Dec 16, 2010, at 4:38 PM, William A. Rowe Jr. wrote:
> On 12/16/2010 3:36 PM, Jim Jagielski wrote:
>>>
>>>> + # where int and long are the same size. Use the longest
>>>> + # type that fits
>>>> + if test "$ac_cv_sizeof_off_t" = "$ac_cv_sizeof_long_long"; then
>>>> + off_t_fmt='#define APR_OFF_T_FMT APR_INT64_T_FMT'
>>>> + off_t_strfn='apr_strtoi64'
>>>
>>> This is bad, no? We don't know that long_long and off_t aren't 128 bytes.
>>> It seems better to use the explicit "ll" format here instead of the value
>>> reserved for 64 bit ints.
>>>
>>
>> All I did was re-arrange the order...
>
> Not arguing, suggesting that the thorough test is either to compare the
> ac_cv_sizeof_off_t to 8, and then use APR_OFF_T_FMT, or failing that, instead
> see if it matches long_long, and use an explicit "ll".
>
Oh, yeah. Well, it's not only the format but
everything as well... After all, if long long is 128bits,
you don't want to use apr_strtoi64 either.
I would suggest that we tackle that issue separately?
Re: [PATCH] Re: [PATCH] %lld support in apr_snprintf()
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
On 12/16/2010 3:36 PM, Jim Jagielski wrote:
>>
>>> + # where int and long are the same size. Use the longest
>>> + # type that fits
>>> + if test "$ac_cv_sizeof_off_t" = "$ac_cv_sizeof_long_long"; then
>>> + off_t_fmt='#define APR_OFF_T_FMT APR_INT64_T_FMT'
>>> + off_t_strfn='apr_strtoi64'
>>
>> This is bad, no? We don't know that long_long and off_t aren't 128 bytes.
>> It seems better to use the explicit "ll" format here instead of the value
>> reserved for 64 bit ints.
>>
>
> All I did was re-arrange the order...
Not arguing, suggesting that the thorough test is either to compare the
ac_cv_sizeof_off_t to 8, and then use APR_OFF_T_FMT, or failing that, instead
see if it matches long_long, and use an explicit "ll".
WDYT?
Re: [PATCH] Re: [PATCH] %lld support in apr_snprintf()
Posted by Jim Jagielski <ji...@apache.org>.
On Dec 16, 2010, at 3:44 PM, William A. Rowe Jr. wrote:
> On 12/16/2010 2:35 PM, Jim Jagielski wrote:
>>
>> On Dec 16, 2010, at 3:23 PM, Jim Jagielski wrote:
>>
>>>
>>> Here is my idea... currently, when looking for sizes
>>> and formats for off_t, we do from smallest to largest
>>> (int -> long -> long long). We also do the same when
>>> checking apr_int64_t as well...
>
>> + # where int and long are the same size. Use the longest
>> + # type that fits
>> + if test "$ac_cv_sizeof_off_t" = "$ac_cv_sizeof_long_long"; then
>> + off_t_fmt='#define APR_OFF_T_FMT APR_INT64_T_FMT'
>> + off_t_strfn='apr_strtoi64'
>
> This is bad, no? We don't know that long_long and off_t aren't 128 bytes.
> It seems better to use the explicit "ll" format here instead of the value
> reserved for 64 bit ints.
>
All I did was re-arrange the order...
Re: [PATCH] Re: [PATCH] %lld support in apr_snprintf()
Posted by Dan Poirier <po...@pobox.com>.
On Thu. 2010-12-16 at 03:44 PM EST, "William A. Rowe Jr." <wr...@rowe-clan.net> wrote:
> On 12/16/2010 2:35 PM, Jim Jagielski wrote:
>>
>> On Dec 16, 2010, at 3:23 PM, Jim Jagielski wrote:
>>
>>>
>>> Here is my idea... currently, when looking for sizes
>>> and formats for off_t, we do from smallest to largest
>>> (int -> long -> long long). We also do the same when
>>> checking apr_int64_t as well...
>
>> + # where int and long are the same size. Use the longest
>> + # type that fits
>> + if test "$ac_cv_sizeof_off_t" = "$ac_cv_sizeof_long_long"; then
>> + off_t_fmt='#define APR_OFF_T_FMT APR_INT64_T_FMT'
>> + off_t_strfn='apr_strtoi64'
>
> This is bad, no? We don't know that long_long and off_t aren't 128 bytes.
> It seems better to use the explicit "ll" format here instead of the value
> reserved for 64 bit ints.
>
> WDYT?
This is a pre-existing problem and not something new with Jim's patch,
right? Maybe we can fix it separately.
Re: [PATCH] Re: [PATCH] %lld support in apr_snprintf()
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
On 12/16/2010 2:35 PM, Jim Jagielski wrote:
>
> On Dec 16, 2010, at 3:23 PM, Jim Jagielski wrote:
>
>>
>> Here is my idea... currently, when looking for sizes
>> and formats for off_t, we do from smallest to largest
>> (int -> long -> long long). We also do the same when
>> checking apr_int64_t as well...
> + # where int and long are the same size. Use the longest
> + # type that fits
> + if test "$ac_cv_sizeof_off_t" = "$ac_cv_sizeof_long_long"; then
> + off_t_fmt='#define APR_OFF_T_FMT APR_INT64_T_FMT'
> + off_t_strfn='apr_strtoi64'
This is bad, no? We don't know that long_long and off_t aren't 128 bytes.
It seems better to use the explicit "ll" format here instead of the value
reserved for 64 bit ints.
WDYT?
Re: [PATCH] Re: [PATCH] %lld support in apr_snprintf()
Posted by Dan Poirier <po...@pobox.com>.
On Fri. 2010-12-17 at 07:29 AM EST, Jim Jagielski <ji...@jaguNET.com> wrote:
>...
> Hmmm.... I cannot recreate that (at least on the 1.4.x branch).
> What happens if you just blank out CFLAGS? The default
> is for universal...
I've been working on trunk, so we might be looking at different things.
There are definitely differences between trunk & 1.4.x in how the
autoconf is set up.
Dan
Re: [PATCH] Re: [PATCH] %lld support in apr_snprintf()
Posted by Jim Jagielski <ji...@jagunet.com>.
On Dec 17, 2010, at 9:12 AM, Graham Leggett wrote:
> On 17 Dec 2010, at 3:53 PM, Jim Jagielski wrote:
>
>> iirc, the concept of using __LP64__ was nixed (iirc, about a year
>> ago I had a large patch set which used that). r824818 and r824762
>>
>> Not sure if that veto is still valid or not.
>
> As I recall, the veto attempted to veto the concept of an Apple "universal binary", something that we've supported without a problem since 2005. I don't think Apple is going to withdraw support for universal binaries on our say so, I recommend we look at it again.
>
I'd agree... ;)
I'll give it a few days and, unless I hear someone re-veto
I'll re-commit those patches...
Re: [PATCH] Re: [PATCH] %lld support in apr_snprintf()
Posted by Graham Leggett <mi...@sharp.fm>.
On 17 Dec 2010, at 3:53 PM, Jim Jagielski wrote:
> iirc, the concept of using __LP64__ was nixed (iirc, about a year
> ago I had a large patch set which used that). r824818 and r824762
>
> Not sure if that veto is still valid or not.
As I recall, the veto attempted to veto the concept of an Apple
"universal binary", something that we've supported without a problem
since 2005. I don't think Apple is going to withdraw support for
universal binaries on our say so, I recommend we look at it again.
Regards,
Graham
--
Re: [PATCH] Re: [PATCH] %lld support in apr_snprintf()
Posted by Jim Jagielski <ji...@jagunet.com>.
On Dec 17, 2010, at 8:32 AM, Graham Leggett wrote:
> On 17 Dec 2010, at 2:33 PM, Jim Jagielski wrote:
>
>> Hold on, yeah, it does seem to still not like universal.
>>
>> But at least this is better than it was...
>
> A while back I asked Apple directly what their solution was, and they said they amend the apr.h after the fact using this:
>
> http://www.opensource.apple.com/source/apr/apr-23/apr/files/fix-apr.h.ed
>
> While amending the apr.h file after the fact is obviously not the solution, picking apart what they've done and building it into the apr.h.in should work.
iirc, the concept of using __LP64__ was nixed (iirc, about a year
ago I had a large patch set which used that). r824818 and r824762
Not sure if that veto is still valid or not.
Re: [PATCH] Re: [PATCH] %lld support in apr_snprintf()
Posted by Jim Jagielski <ji...@apache.org>.
On Mar 8, 2011, at 4:43 PM, Jeff Trawick wrote:
> On Tue, Mar 8, 2011 at 1:42 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>>
>> On Mar 8, 2011, at 9:49 AM, Hyrum K. Wright wrote:
>>
>>> On Fri, Dec 17, 2010 at 10:23 AM, Jim Jagielski <ji...@jagunet.com> wrote:
>>>>
>>>> On Dec 17, 2010, at 11:13 AM, William A. Rowe Jr. wrote:
>>>>
>>>>> On 12/17/2010 7:32 AM, Graham Leggett wrote:
>>>>>>
>>>>>> While amending the apr.h file after the fact is obviously not the solution, picking apart
>>>>>> what they've done and building it into the apr.h.in should work.
>>>>>
>>>>> Right, Fred had submitted this to the list. Joe had vetoed.
>>>>>
>>>>
>>>> I have an updated patch ready to go... Awaiting approval
>>>> to commit ;)
>>>
>>> Did this patch ever get committed? We're still getting reports of
>>> folks in Subversion-land who are experiencing this crash with APR on
>>> Mac OS X.
>>>
>>
>> It's committed but there have been no releases of APR yet since it
>> was committed...
>
> worth adding to CHANGES, no?
>
Yeah, good idea ;)
Will do.
Re: [PATCH] Re: [PATCH] %lld support in apr_snprintf()
Posted by Jeff Trawick <tr...@gmail.com>.
On Tue, Mar 8, 2011 at 1:42 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>
> On Mar 8, 2011, at 9:49 AM, Hyrum K. Wright wrote:
>
>> On Fri, Dec 17, 2010 at 10:23 AM, Jim Jagielski <ji...@jagunet.com> wrote:
>>>
>>> On Dec 17, 2010, at 11:13 AM, William A. Rowe Jr. wrote:
>>>
>>>> On 12/17/2010 7:32 AM, Graham Leggett wrote:
>>>>>
>>>>> While amending the apr.h file after the fact is obviously not the solution, picking apart
>>>>> what they've done and building it into the apr.h.in should work.
>>>>
>>>> Right, Fred had submitted this to the list. Joe had vetoed.
>>>>
>>>
>>> I have an updated patch ready to go... Awaiting approval
>>> to commit ;)
>>
>> Did this patch ever get committed? We're still getting reports of
>> folks in Subversion-land who are experiencing this crash with APR on
>> Mac OS X.
>>
>
> It's committed but there have been no releases of APR yet since it
> was committed...
worth adding to CHANGES, no?
Re: [PATCH] Re: [PATCH] %lld support in apr_snprintf()
Posted by Jim Jagielski <ji...@jagunet.com>.
On Mar 8, 2011, at 9:49 AM, Hyrum K. Wright wrote:
> On Fri, Dec 17, 2010 at 10:23 AM, Jim Jagielski <ji...@jagunet.com> wrote:
>>
>> On Dec 17, 2010, at 11:13 AM, William A. Rowe Jr. wrote:
>>
>>> On 12/17/2010 7:32 AM, Graham Leggett wrote:
>>>>
>>>> While amending the apr.h file after the fact is obviously not the solution, picking apart
>>>> what they've done and building it into the apr.h.in should work.
>>>
>>> Right, Fred had submitted this to the list. Joe had vetoed.
>>>
>>
>> I have an updated patch ready to go... Awaiting approval
>> to commit ;)
>
> Did this patch ever get committed? We're still getting reports of
> folks in Subversion-land who are experiencing this crash with APR on
> Mac OS X.
>
It's committed but there have been no releases of APR yet since it
was committed...
Re: [PATCH] Re: [PATCH] %lld support in apr_snprintf()
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Dec 17, 2010, at 11:13 AM, William A. Rowe Jr. wrote:
> On 12/17/2010 7:32 AM, Graham Leggett wrote:
>>
>> While amending the apr.h file after the fact is obviously not the solution, picking apart
>> what they've done and building it into the apr.h.in should work.
>
> Right, Fred had submitted this to the list. Joe had vetoed.
>
I have an updated patch ready to go... Awaiting approval
to commit ;)
Re: [PATCH] Re: [PATCH] %lld support in apr_snprintf()
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
On 12/17/2010 7:32 AM, Graham Leggett wrote:
>
> While amending the apr.h file after the fact is obviously not the solution, picking apart
> what they've done and building it into the apr.h.in should work.
Right, Fred had submitted this to the list. Joe had vetoed.
/me Grabs for the popcorn :)
Re: [PATCH] Re: [PATCH] %lld support in apr_snprintf()
Posted by Graham Leggett <mi...@sharp.fm>.
On 17 Dec 2010, at 2:33 PM, Jim Jagielski wrote:
> Hold on, yeah, it does seem to still not like universal.
>
> But at least this is better than it was...
A while back I asked Apple directly what their solution was, and they
said they amend the apr.h after the fact using this:
http://www.opensource.apple.com/source/apr/apr-23/apr/files/fix-apr.h.ed
While amending the apr.h file after the fact is obviously not the
solution, picking apart what they've done and building it into the
apr.h.in should work.
Regards,
Graham
--
Re: [PATCH] Re: [PATCH] %lld support in apr_snprintf()
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Dec 17, 2010, at 7:29 AM, Jim Jagielski wrote:
>>
>> Example: if I build universal (CFLAGS='-arch i386 -arch x86_64'), the compile
>> fails in strings/apr_snprintf.c:
>>
>
> Hmmm.... I cannot recreate that (at least on the 1.4.x branch).
> What happens if you just blank out CFLAGS? The default
> is for universal...
>
Hold on, yeah, it does seem to still not like universal.
But at least this is better than it was...
Re: [PATCH] Re: [PATCH] %lld support in apr_snprintf()
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Dec 17, 2010, at 6:58 AM, Dan Poirier wrote:
> On Thu. 2010-12-16 at 03:35 PM EST, Jim Jagielski <ji...@jaguNET.com> wrote:
>
>> On Dec 16, 2010, at 3:23 PM, Jim Jagielski wrote:
>>
>>>
>>> Here is my idea... currently, when looking for sizes
>>> and formats for off_t, we do from smallest to largest
>>> (int -> long -> long long). We also do the same when
>>> checking apr_int64_t as well...
>>>
>>> What if we do the reverse? What if instead of finding
>>> the smallest that is the right size, we find the first,
>>> starting with the longest?
>>>
>>> I'm trying that out now, so we'll see...
>>>
>>
>> hey.... this looks promising! With this, APR and httpd
>> builds with no "warning ... expect" errors at all, and
>> passes the test framework, no matter is built forcing 64bit,
>> 32bit *or universal*!
>>
>> Please check over because I hope to commit sometimes
>> tomorrow!!
>
> I think universal is going to take more work. Right now, configure runs
> little test programs once, either 32-bit or 64-bit, and records the
> results in apr.h and other places. Those answers can't always be right
> for both 32-bit and 64-bit. The header files will need to have some
> conditionals to use the right answers depending on which way the code is
> being compiled. Other programs obviously manage to do this. Maybe it's
> simpler than it seems to me.
>
> Example: if I build universal (CFLAGS='-arch i386 -arch x86_64'), the compile
> fails in strings/apr_snprintf.c:
>
Hmmm.... I cannot recreate that (at least on the 1.4.x branch).
What happens if you just blank out CFLAGS? The default
is for universal...
> strings/apr_snprintf.c: In function ‘conv_os_thread_t’:
> strings/apr_snprintf.c:500: error: duplicate case value
> strings/apr_snprintf.c:498: error: previously used here
> strings/apr_snprintf.c: In function ‘conv_os_thread_t_hex’:
> strings/apr_snprintf.c:671: error: duplicate case value
> strings/apr_snprintf.c:669: error: previously used here
>
> because sizeof(apr_int32_t) is apparently the same as
> sizeof(apr_int64_t), which is obviously wrong. Here's apr.h:
>
> typedef int apr_int32_t;
> typedef unsigned int apr_uint32_t;
>
> typedef long apr_int64_t;
> typedef unsigned long apr_uint64_t;
>
> (What's interesting is that if I build just 32-bit or just 64-bit, that
> compile does not fail. I don't know why; maybe the tests that set
> apr_int32_t and apr_int64_t respond in different ways to universal
> builds.)
>
> But all this is beside the point of the current issue. I think if we
> can get apr to build & test okay separately, 32-bit or 64-bit, that'll
> be real worthwhile progress. Universal can wait for another day :-)
>
> Dan
>
Re: [PATCH] Re: [PATCH] %lld support in apr_snprintf()
Posted by Dan Poirier <po...@pobox.com>.
On Thu. 2010-12-16 at 03:35 PM EST, Jim Jagielski <ji...@jaguNET.com> wrote:
> On Dec 16, 2010, at 3:23 PM, Jim Jagielski wrote:
>
>>
>> Here is my idea... currently, when looking for sizes
>> and formats for off_t, we do from smallest to largest
>> (int -> long -> long long). We also do the same when
>> checking apr_int64_t as well...
>>
>> What if we do the reverse? What if instead of finding
>> the smallest that is the right size, we find the first,
>> starting with the longest?
>>
>> I'm trying that out now, so we'll see...
>>
>
> hey.... this looks promising! With this, APR and httpd
> builds with no "warning ... expect" errors at all, and
> passes the test framework, no matter is built forcing 64bit,
> 32bit *or universal*!
>
> Please check over because I hope to commit sometimes
> tomorrow!!
I think universal is going to take more work. Right now, configure runs
little test programs once, either 32-bit or 64-bit, and records the
results in apr.h and other places. Those answers can't always be right
for both 32-bit and 64-bit. The header files will need to have some
conditionals to use the right answers depending on which way the code is
being compiled. Other programs obviously manage to do this. Maybe it's
simpler than it seems to me.
Example: if I build universal (CFLAGS='-arch i386 -arch x86_64'), the compile
fails in strings/apr_snprintf.c:
strings/apr_snprintf.c: In function ‘conv_os_thread_t’:
strings/apr_snprintf.c:500: error: duplicate case value
strings/apr_snprintf.c:498: error: previously used here
strings/apr_snprintf.c: In function ‘conv_os_thread_t_hex’:
strings/apr_snprintf.c:671: error: duplicate case value
strings/apr_snprintf.c:669: error: previously used here
because sizeof(apr_int32_t) is apparently the same as
sizeof(apr_int64_t), which is obviously wrong. Here's apr.h:
typedef int apr_int32_t;
typedef unsigned int apr_uint32_t;
typedef long apr_int64_t;
typedef unsigned long apr_uint64_t;
(What's interesting is that if I build just 32-bit or just 64-bit, that
compile does not fail. I don't know why; maybe the tests that set
apr_int32_t and apr_int64_t respond in different ways to universal
builds.)
But all this is beside the point of the current issue. I think if we
can get apr to build & test okay separately, 32-bit or 64-bit, that'll
be real worthwhile progress. Universal can wait for another day :-)
Dan
[PATCH] Re: [PATCH] %lld support in apr_snprintf()
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Dec 16, 2010, at 3:23 PM, Jim Jagielski wrote:
>
> Here is my idea... currently, when looking for sizes
> and formats for off_t, we do from smallest to largest
> (int -> long -> long long). We also do the same when
> checking apr_int64_t as well...
>
> What if we do the reverse? What if instead of finding
> the smallest that is the right size, we find the first,
> starting with the longest?
>
> I'm trying that out now, so we'll see...
>
hey.... this looks promising! With this, APR and httpd
builds with no "warning ... expect" errors at all, and
passes the test framework, no matter is built forcing 64bit,
32bit *or universal*!
Please check over because I hope to commit sometimes
tomorrow!!
Index: configure.in
===================================================================
--- configure.in (revision 1050141)
+++ configure.in (working copy)
@@ -1526,24 +1526,15 @@
fi
# Now we need to find what apr_int64_t (sizeof == 8) will be.
# The first match is our preference.
-if test "$ac_cv_sizeof_int" = "8"; then
- int64_literal='#define APR_INT64_C(val) (val)'
- uint64_literal='#define APR_UINT64_C(val) (val##U)'
- int64_t_fmt='#define APR_INT64_T_FMT "d"'
- uint64_t_fmt='#define APR_UINT64_T_FMT "u"'
- uint64_t_hex_fmt='#define APR_UINT64_T_HEX_FMT "x"'
- int64_value="int"
- long_value=int
- int64_strfn="strtoi"
-elif test "$ac_cv_sizeof_long" = "8"; then
- int64_literal='#define APR_INT64_C(val) (val##L)'
- uint64_literal='#define APR_UINT64_C(val) (val##UL)'
- int64_t_fmt='#define APR_INT64_T_FMT "ld"'
- uint64_t_fmt='#define APR_UINT64_T_FMT "lu"'
- uint64_t_hex_fmt='#define APR_UINT64_T_HEX_FMT "lx"'
- int64_value="long"
- long_value=long
- int64_strfn="strtol"
+if test "$ac_cv_sizeof_longlong" = "8"; then
+ int64_literal='#define APR_INT64_C(val) (val##LL)'
+ uint64_literal='#define APR_UINT64_C(val) (val##ULL)'
+ int64_t_fmt='#define APR_INT64_T_FMT "qd"'
+ uint64_t_fmt='#define APR_UINT64_T_FMT "qu"'
+ uint64_t_hex_fmt='#define APR_UINT64_T_HEX_FMT "qx"'
+ int64_value="__int64"
+ long_value="__int64"
+ int64_strfn="strtoll"
elif test "$ac_cv_sizeof_long_long" = "8"; then
int64_literal='#define APR_INT64_C(val) (val##LL)'
uint64_literal='#define APR_UINT64_C(val) (val##ULL)'
@@ -1557,15 +1548,24 @@
int64_value="long long"
long_value="long long"
int64_strfn="strtoll"
-elif test "$ac_cv_sizeof_longlong" = "8"; then
- int64_literal='#define APR_INT64_C(val) (val##LL)'
- uint64_literal='#define APR_UINT64_C(val) (val##ULL)'
- int64_t_fmt='#define APR_INT64_T_FMT "qd"'
- uint64_t_fmt='#define APR_UINT64_T_FMT "qu"'
- uint64_t_hex_fmt='#define APR_UINT64_T_HEX_FMT "qx"'
- int64_value="__int64"
- long_value="__int64"
- int64_strfn="strtoll"
+elif test "$ac_cv_sizeof_long" = "8"; then
+ int64_literal='#define APR_INT64_C(val) (val##L)'
+ uint64_literal='#define APR_UINT64_C(val) (val##UL)'
+ int64_t_fmt='#define APR_INT64_T_FMT "ld"'
+ uint64_t_fmt='#define APR_UINT64_T_FMT "lu"'
+ uint64_t_hex_fmt='#define APR_UINT64_T_HEX_FMT "lx"'
+ int64_value="long"
+ long_value=long
+ int64_strfn="strtol"
+elif test "$ac_cv_sizeof_int" = "8"; then
+ int64_literal='#define APR_INT64_C(val) (val)'
+ uint64_literal='#define APR_UINT64_C(val) (val##U)'
+ int64_t_fmt='#define APR_INT64_T_FMT "d"'
+ uint64_t_fmt='#define APR_UINT64_T_FMT "u"'
+ uint64_t_hex_fmt='#define APR_UINT64_T_HEX_FMT "x"'
+ int64_value="int"
+ long_value=int
+ int64_strfn="strtoi"
else
# int64_literal may be overriden if your compiler thinks you have
# a 64-bit value but APR does not agree.
@@ -1749,16 +1749,17 @@
elif test "$ac_cv_type_off_t" = "yes"; then
off_t_value=off_t
# off_t is more commonly a long than an int; prefer that case
- # where int and long are the same size.
- if test "$ac_cv_sizeof_off_t" = "$ac_cv_sizeof_long"; then
+ # where int and long are the same size. Use the longest
+ # type that fits
+ if test "$ac_cv_sizeof_off_t" = "$ac_cv_sizeof_long_long"; then
+ off_t_fmt='#define APR_OFF_T_FMT APR_INT64_T_FMT'
+ off_t_strfn='apr_strtoi64'
+ elif test "$ac_cv_sizeof_off_t" = "$ac_cv_sizeof_long"; then
off_t_fmt='#define APR_OFF_T_FMT "ld"'
off_t_strfn='strtol'
elif test "$ac_cv_sizeof_off_t" = "$ac_cv_sizeof_int"; then
off_t_fmt='#define APR_OFF_T_FMT "d"'
off_t_strfn='strtoi'
- elif test "$ac_cv_sizeof_off_t" = "$ac_cv_sizeof_long_long"; then
- off_t_fmt='#define APR_OFF_T_FMT APR_INT64_T_FMT'
- off_t_strfn='apr_strtoi64'
else
AC_ERROR([could not determine the size of off_t])
fi
Re: [PATCH] %lld support in apr_snprintf()
Posted by Jim Jagielski <ji...@jaguNET.com>.
Here is my idea... currently, when looking for sizes
and formats for off_t, we do from smallest to largest
(int -> long -> long long). We also do the same when
checking apr_int64_t as well...
What if we do the reverse? What if instead of finding
the smallest that is the right size, we find the first,
starting with the longest?
I'm trying that out now, so we'll see...
Re: [PATCH] %lld support in apr_snprintf()
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Dec 15, 2010, at 5:11 PM, Dan Poirier wrote:
> On Mon. 2010-11-08 at 10:27 AM EST, Jim Jagielski <ji...@jaguNET.com> wrote:
>
>> On Nov 7, 2010, at 7:42 PM, Jeff Trawick wrote:
>>
>>> On Sun, Nov 7, 2010 at 6:51 PM, Chris Knight
>>> <Ch...@nasa.gov> wrote:
>>>> Exactly, the problem only appears on 64-bit Snow Leopard. See my patch in Bugzilla, which I've verified. (Unsure if the below would also work, been a long time since I diagnosed.)
>>>
>>> What I understood was that %lld is supposed to work, independent of
>>> which APR_*_FMT defines use %lld.
>>>
>>> I haven't tried the other patches on Leopard-64 to see which help there.
>>>
>>
>> If one forces *just* 64bit, then, afaict, the patch is not needed.
>> It's only if one builds APR with both i386 and x86_64 that
>> things break...
>
> That's not my experience. If I build just 64bit (CC="gcc -arch
> x86_64"), testfmt fails on APR_OFF_T_FMT. (And httpd fails most of its
> byterange tests.)
>
> With Sander's patch to notice the second "l" in "%ll?", that test
> works but another one, using APR_UINT64_T_FMT, fails. I haven't dug
> into that one yet.
Maybe we should make 64bit the default for APR (and httpd)
and have the weirdness only when 32bit is used. In other words,
we patch APR that the expectation is that 10.6 will be 64bit
and when compiled as 64bit, all tests and builds are clean.
It's only when 32bit, or, even-worse, a universal build, is
done that we get the errors.
It sure would be nice if when Apple releases 10.7 (and
the complimentary XCode) that this issue goes away...
Re: [PATCH] %lld support in apr_snprintf()
Posted by Dan Poirier <po...@pobox.com>.
On Mon. 2010-11-08 at 10:27 AM EST, Jim Jagielski <ji...@jaguNET.com> wrote:
> On Nov 7, 2010, at 7:42 PM, Jeff Trawick wrote:
>
>> On Sun, Nov 7, 2010 at 6:51 PM, Chris Knight
>> <Ch...@nasa.gov> wrote:
>>> Exactly, the problem only appears on 64-bit Snow Leopard. See my patch in Bugzilla, which I've verified. (Unsure if the below would also work, been a long time since I diagnosed.)
>>
>> What I understood was that %lld is supposed to work, independent of
>> which APR_*_FMT defines use %lld.
>>
>> I haven't tried the other patches on Leopard-64 to see which help there.
>>
>
> If one forces *just* 64bit, then, afaict, the patch is not needed.
> It's only if one builds APR with both i386 and x86_64 that
> things break...
That's not my experience. If I build just 64bit (CC="gcc -arch
x86_64"), testfmt fails on APR_OFF_T_FMT. (And httpd fails most of its
byterange tests.)
With Sander's patch to notice the second "l" in "%ll?", that test
works but another one, using APR_UINT64_T_FMT, fails. I haven't dug
into that one yet.
Dan
Re: [PATCH] %lld support in apr_snprintf()
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Nov 7, 2010, at 7:42 PM, Jeff Trawick wrote:
> On Sun, Nov 7, 2010 at 6:51 PM, Chris Knight
> <Ch...@nasa.gov> wrote:
>> Exactly, the problem only appears on 64-bit Snow Leopard. See my patch in Bugzilla, which I've verified. (Unsure if the below would also work, been a long time since I diagnosed.)
>
> What I understood was that %lld is supposed to work, independent of
> which APR_*_FMT defines use %lld.
>
> I haven't tried the other patches on Leopard-64 to see which help there.
>
If one forces *just* 64bit, then, afaict, the patch is not needed.
It's only if one builds APR with both i386 and x86_64 that
things break...
Re: [PATCH] %lld support in apr_snprintf()
Posted by Jeff Trawick <tr...@gmail.com>.
On Sun, Nov 7, 2010 at 6:51 PM, Chris Knight
<Ch...@nasa.gov> wrote:
> Exactly, the problem only appears on 64-bit Snow Leopard. See my patch in Bugzilla, which I've verified. (Unsure if the below would also work, been a long time since I diagnosed.)
What I understood was that %lld is supposed to work, independent of
which APR_*_FMT defines use %lld.
I haven't tried the other patches on Leopard-64 to see which help there.
Re: [PATCH] %lld support in apr_snprintf()
Posted by Chris Knight <Ch...@nasa.gov>.
Exactly, the problem only appears on 64-bit Snow Leopard. See my patch in Bugzilla, which I've verified. (Unsure if the below would also work, been a long time since I diagnosed.)
On Nov 7, 2010, at 3:36 PM, Jeff Trawick wrote:
> On Sun, Nov 7, 2010 at 5:18 PM, Jeff Trawick <tr...@gmail.com> wrote:
>> On Sun, Nov 7, 2010 at 5:07 PM, Sander Temme <sa...@temme.net> wrote:
>>>
>>> On Nov 4, 2010, at 12:15 PM, William A. Rowe Jr. wrote:
>>>
>>>> Looks good here.
>>>
>>> If folks find this unproblematic, can someone please commit it? I don't have karma here.
>>>
>>> Thanks,
>>
>> looks fine to me; starting to try it out now
>
> The patch is apparently not needed to make %lld work in 32-bit mode on
> Leopard since APR_INT64_T_FMT is %lld.
> It didn't make %lld work in 64-bit mode.
>
> What are the cases where you see this help out?
>
> I guess you're on Snow Leopard?
>
> Here's what I tried in 64-bit mode:
>
> Index: strings/apr_snprintf.c
> ===================================================================
> --- strings/apr_snprintf.c (revision 1032408)
> +++ strings/apr_snprintf.c (working copy)
> @@ -832,6 +832,11 @@
> else if (*fmt == 'l') {
> var_type = IS_LONG;
> fmt++;
> + /* Catch the %lld type modifier for long long and its ilk */
> + if (*fmt == 'l') {
> + var_type = IS_QUAD;
> + fmt++;
> + }
> }
> else if (*fmt == 'h') {
> var_type = IS_SHORT;
>
>
> ndex: testfmt.c
> ===================================================================
> --- testfmt.c (revision 1032408)
> +++ testfmt.c (working copy)
> @@ -126,6 +126,9 @@
>
> apr_snprintf(buf, sizeof buf, "%" APR_INT64_T_FMT, ibig);
> ABTS_STR_EQUAL(tc, "-314159265358979323", buf);
> +
> + apr_snprintf(buf, sizeof buf, "%lld", ibig);
> + ABTS_STR_EQUAL(tc, "-314159265358979323", buf);
> }
>
> static void error_fmt(abts_case *tc, void *data)
>
> ./testall -v testfmt
> testfmt : /Line 131: expected <-314159265358979323>, but saw <%ld>
> FAILED 1 of 10
> Failed Tests Total Fail Failed %
> ===================================================
> testfmt 10 1 10.00%
Re: [PATCH] %lld support in apr_snprintf()
Posted by Jeff Trawick <tr...@gmail.com>.
On Sun, Nov 7, 2010 at 5:18 PM, Jeff Trawick <tr...@gmail.com> wrote:
> On Sun, Nov 7, 2010 at 5:07 PM, Sander Temme <sa...@temme.net> wrote:
>>
>> On Nov 4, 2010, at 12:15 PM, William A. Rowe Jr. wrote:
>>
>>> Looks good here.
>>
>> If folks find this unproblematic, can someone please commit it? I don't have karma here.
>>
>> Thanks,
>
> looks fine to me; starting to try it out now
The patch is apparently not needed to make %lld work in 32-bit mode on
Leopard since APR_INT64_T_FMT is %lld.
It didn't make %lld work in 64-bit mode.
What are the cases where you see this help out?
I guess you're on Snow Leopard?
Here's what I tried in 64-bit mode:
Index: strings/apr_snprintf.c
===================================================================
--- strings/apr_snprintf.c (revision 1032408)
+++ strings/apr_snprintf.c (working copy)
@@ -832,6 +832,11 @@
else if (*fmt == 'l') {
var_type = IS_LONG;
fmt++;
+ /* Catch the %lld type modifier for long long and its ilk */
+ if (*fmt == 'l') {
+ var_type = IS_QUAD;
+ fmt++;
+ }
}
else if (*fmt == 'h') {
var_type = IS_SHORT;
ndex: testfmt.c
===================================================================
--- testfmt.c (revision 1032408)
+++ testfmt.c (working copy)
@@ -126,6 +126,9 @@
apr_snprintf(buf, sizeof buf, "%" APR_INT64_T_FMT, ibig);
ABTS_STR_EQUAL(tc, "-314159265358979323", buf);
+
+ apr_snprintf(buf, sizeof buf, "%lld", ibig);
+ ABTS_STR_EQUAL(tc, "-314159265358979323", buf);
}
static void error_fmt(abts_case *tc, void *data)
./testall -v testfmt
testfmt : /Line 131: expected <-314159265358979323>, but saw <%ld>
FAILED 1 of 10
Failed Tests Total Fail Failed %
===================================================
testfmt 10 1 10.00%
Re: [PATCH] %lld support in apr_snprintf()
Posted by Jeff Trawick <tr...@gmail.com>.
On Sun, Nov 7, 2010 at 5:07 PM, Sander Temme <sa...@temme.net> wrote:
>
> On Nov 4, 2010, at 12:15 PM, William A. Rowe Jr. wrote:
>
>> Looks good here.
>
> If folks find this unproblematic, can someone please commit it? I don't have karma here.
>
> Thanks,
looks fine to me; starting to try it out now
Re: [PATCH] %lld support in apr_snprintf()
Posted by Sander Temme <sa...@temme.net>.
On Nov 4, 2010, at 12:15 PM, William A. Rowe Jr. wrote:
> Looks good here.
If folks find this unproblematic, can someone please commit it? I don't have karma here.
Thanks,
S.
--
sander@temme.net http://www.temme.net/sander/
PGP FP: FC5A 6FC6 2E25 2DFD 8007 EE23 9BB8 63B0 F51B B88A
View my availability: http://tungle.me/sctemme
Re: [PATCH] %lld support in apr_snprintf()
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
On 11/4/2010 2:46 PM, Sander Temme wrote:
> Folks,
>
> I was seeing test failures on Darwin in both the APR testsuite and httpd perl-framework. The %lld sprintf format character was incorrectly parsed, and "%ld" written instead of the substituted value.
>
> This small patch against APR trunk fixes that:
>
> Index: strings/apr_snprintf.c
> ===================================================================
> --- strings/apr_snprintf.c (revision 1031121)
> +++ strings/apr_snprintf.c (working copy)
> @@ -832,6 +832,11 @@
> else if (*fmt == 'l') {
> var_type = IS_LONG;
> fmt++;
> + /* Catch the %lld type modifier for long long and its ilk */
> + if (*fmt == 'l') {
> + var_type = IS_QUAD;
> + fmt++;
> + }
> }
> else if (*fmt == 'h') {
> var_type = IS_SHORT;
Looks good here.