You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Hudson <gh...@MIT.EDU> on 2004/01/16 18:30:29 UTC
Re: svn commit: r8343 - trunk/subversion/libsvn_ra_svn
On Fri, 2004-01-16 at 13:20, ghudson@tigris.org wrote:
> * subversion/libsvn_ra_svn/cram.c (compute_digest): Use an unsigned
> char array for the secret. Patch from Philip Martin.
The bitwise operations performed on secret are all xors, so there should
be no semantic problem with them being (possibly) signed char. So, it's
mainly an issue of warnings with pickier compilers. Do people think
that's important enough for a 1.0 pullup?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r8343 - trunk/subversion/libsvn_ra_svn
Posted by Branko Čibej <br...@xbc.nu>.
Philip Martin wrote:
>Branko Čibej <br...@xbc.nu> writes:
>
>
>>I'd say this is a defect in the standard. According to these two
>>paragraphs, you _cannot_ write portable code that uses signals, and xlc
>>demonstrates this.
>>
>>
>
>6.7.3/4 talking about type qualifiers
>
> "If the same qualifier appears more than once in the same
> specifier-qualifier-list, either directly or via one or more
> typedefs, the behaviour is the same as if it appeared once"
>
>That's the bit that xlc gets wrong. I didn't find that the first time
>I looked (but then I'm not really a language lawyer, I just play at
>the weekend :)
>
>
Oooh, you're right! I wonder if this wording existed in the C90
standard. Anyway, this is a bug to be filed against xlc, not an APR
problem (although realistically, APR can fix it more quickly).
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r8343 - trunk/subversion/libsvn_ra_svn
Posted by Philip Martin <ph...@codematters.co.uk>.
Branko Äibej <br...@xbc.nu> writes:
> Philip Martin wrote:
>
>>7.14/2 talking about <signal.h> states
>>
>> "the type defined is sig_atomic_t which is the (possibly
>> volatile-qualifed) integer type"
>>
>>so a volatile qualifer in signal.h is permitted.
>>
>>7.14.1.1/5 talking about signal handlers
>>
>> "the behaviour is undefined if the signal handler refers to any
>> object with static storage duration other than be assigning to an
>> object declared as volatile sig_atomic_t"
>>
>>Since one cannot know whether signal.h provides a volatile qualifier
>>the only way to write portable code is to include the volatile
>>qualifier in the declaration in the program. To leave it out and rely
>>on the qualifier in signal.h would result in code that is invalid when
>>using a different implementation (say the next compiler upgrade).
>>
>>So my conclusion would be that signal.h is permitted to include a
>>volatile qualifier, but if it does it must allow a duplicate qualifier
>>from the program.
>>
> I'd say this is a defect in the standard. According to these two
> paragraphs, you _cannot_ write portable code that uses signals, and xlc
> demonstrates this.
6.7.3/4 talking about type qualifiers
"If the same qualifier appears more than once in the same
specifier-qualifier-list, either directly or via one or more
typedefs, the behaviour is the same as if it appeared once"
That's the bit that xlc gets wrong. I didn't find that the first time
I looked (but then I'm not really a language lawyer, I just play at
the weekend :)
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r8343 - trunk/subversion/libsvn_ra_svn
Posted by Branko Čibej <br...@xbc.nu>.
Philip Martin wrote:
>Travis P <sv...@castle.fastmail.fm> writes:
>
>
>
>>If the standard is explicit (if you have a section reference that'd be
>>great, if not I'll just look for it when I can get my hands on a
>>copy), then I'll see if I can't feed this back to the xlc developers
>>for them to fix.
>>
>>
>
>7.14/2 talking about <signal.h> states
>
> "the type defined is sig_atomic_t which is the (possibly
> volatile-qualifed) integer type"
>
>so a volatile qualifer in signal.h is permitted.
>
>7.14.1.1/5 talking about signal handlers
>
> "the behaviour is undefined if the signal handler refers to any
> object with static storage duration other than be assigning to an
> object declared as volatile sig_atomic_t"
>
>Since one cannot know whether signal.h provides a volatile qualifier
>the only way to write portable code is to include the volatile
>qualifier in the declaration in the program. To leave it out and rely
>on the qualifier in signal.h would result in code that is invalid when
>using a different implementation (say the next compiler upgrade).
>
>So my conclusion would be that signal.h is permitted to include a
>volatile qualifier, but if it does it must allow a duplicate qualifier
>from the program.
>
>
I'd say this is a defect in the standard. According to these two
paragraphs, you _cannot_ write portable code that uses signals, and xlc
demonstrates this.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r8343 - trunk/subversion/libsvn_ra_svn
Posted by Philip Martin <ph...@codematters.co.uk>.
Travis P <sv...@castle.fastmail.fm> writes:
> If the standard is explicit (if you have a section reference that'd be
> great, if not I'll just look for it when I can get my hands on a
> copy), then I'll see if I can't feed this back to the xlc developers
> for them to fix.
7.14/2 talking about <signal.h> states
"the type defined is sig_atomic_t which is the (possibly
volatile-qualifed) integer type"
so a volatile qualifer in signal.h is permitted.
7.14.1.1/5 talking about signal handlers
"the behaviour is undefined if the signal handler refers to any
object with static storage duration other than be assigning to an
object declared as volatile sig_atomic_t"
Since one cannot know whether signal.h provides a volatile qualifier
the only way to write portable code is to include the volatile
qualifier in the declaration in the program. To leave it out and rely
on the qualifier in signal.h would result in code that is invalid when
using a different implementation (say the next compiler upgrade).
So my conclusion would be that signal.h is permitted to include a
volatile qualifier, but if it does it must allow a duplicate qualifier
from the program.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r8343 - trunk/subversion/libsvn_ra_svn
Posted by Travis P <sv...@castle.fastmail.fm>.
On Jan 16, 2004, at 5:19 PM, Philip Martin wrote:
> [Apologies if this appears twice, but I think I lost the first one.]
>
> kfogel@collab.net writes:
>
>> Greg Hudson <gh...@MIT.EDU> writes:
>>>> * subversion/libsvn_ra_svn/cram.c (compute_digest): Use an unsigned
>>>> char array for the secret. Patch from Philip Martin.
>>>
>>> The bitwise operations performed on secret are all xors, so there
>>> should
>>> be no semantic problem with them being (possibly) signed char. So,
>>> it's
>>> mainly an issue of warnings with pickier compilers. Do people think
>>> that's important enough for a 1.0 pullup?
>>
>> Personally, I'd rather not. We're already accumulating a lot of
>> little proposed changes late in the game. This one doesn't seem that
>> compelling, given that all we're preventing is a compile warning on
>> some platforms.
>
> It's a bit more than that, the report that led to this patch involved
> using the xlc compiler on AIX, and it was a compiler *error*. xlc
> also has the "volatile sig_atomic_t" problem for which I don't have a
> ready solution, other than suggesting that those that encounter it
> delete volatile from the code. I suspect neither problem affects gcc
> on AIX.
Yes, it was an error. Not a warning. Whether it should be so or not
may be debated, but my experience has led me to believe that the xlc
developers are interested and somewhat strict in coding to the
standard.
Here's a tiny test program that invokes the situation in question under
the various language variations available from the compiler:
====== a.c ======
void test(unsigned char uca[10]) { }
int main() {
char sca[10];
test(sca);
return 0;
}
====== ======
% xlc -qlanglvl=extc89 a.c
% xlc -qlanglvl=stdc89 a.c
"a.c", line 7.8: 1506-280 (E) Function argument assignment between
types "unsigned char*" and "char*" is not allowed.
% xlc -qlanglvl=extc99 a.c
% xlc -qlanglvl=stdc99 a.c
"a.c", line 7.8: 1506-280 (E) Function argument assignment between
types "unsigned char*" and "char*" is not allowed.
% xlc -qlanglvl=ansi a.c
"a.c", line 7.8: 1506-280 (E) Function argument assignment between
types "unsigned char*" and "char*" is not allowed.
% xlc -qlanglvl=saal2 a.c
"a.c", line 7.8: 1506-280 (E) Function argument assignment between
types "unsigned char*" and "char*" is not allowed.
% xlc -qlanglvl=saa a.c
"a.c", line 7.8: 1506-280 (E) Function argument assignment between
types "unsigned char*" and "char*" is not allowed.
% xlc -qlanglvl=extended a.c
% xlc -qlanglvl=classic a.c
%
It does appear that the xlc developers believe that when adhering to
the standard, the condition in question may cause an error (though we
don't know whether such a type mismatch MUST be an error or can be an
error at the implementors discretion). I don't have the standard
available at the moment to cite one way or another.
As you can also see, there are compiler options available to make it
more lenient. I chose "xlc" because if I didn't select a compiler,
configure was picking up an older (2.x) gcc compiler. I could and
maybe should have chosen "cc" which is xlc using one of the more
lenient language options.
On Jan 16, 2004, at 11:38 AM, Philip Martin wrote:
> Hmm, again. I think this is a real AIX bug. The C standard is
> explicit, we need to use "volatile sig_atomic_t". I see that gcc's
> fixincl removes volatile from the shadow sys/signal.h, but says that
> only some versions of AIX are affected. We need to be very careful
> not to remove volatile from our code in cases where there is no
> duplicate. Perhaps we could use an autoconf macro. Or we could use
> an integer type directly, but that's no more portable than not using
> volatile. Or we could leave it as it is, but add a comment to the
> code saying that if the error occurs then volatile should be removed.
I'll have do some more investigation into the "volatile sig_atomic_t"
problem. It does respond the same way as the type problem above to
the language options: it's an error with the default strict "xlc"
settings but the more lenient "cc" settings don't cause an error or
even a warning.
If the standard is explicit (if you have a section reference that'd be
great, if not I'll just look for it when I can get my hands on a copy),
then I'll see if I can't feed this back to the xlc developers for them
to fix.
-Travis Pouarz
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r8343 - trunk/subversion/libsvn_ra_svn
Posted by Philip Martin <ph...@codematters.co.uk>.
[Appologies if this appears twice, but I think I lost the first one.]
kfogel@collab.net writes:
> Greg Hudson <gh...@MIT.EDU> writes:
>> > * subversion/libsvn_ra_svn/cram.c (compute_digest): Use an unsigned
>> > char array for the secret. Patch from Philip Martin.
>>
>> The bitwise operations performed on secret are all xors, so there should
>> be no semantic problem with them being (possibly) signed char. So, it's
>> mainly an issue of warnings with pickier compilers. Do people think
>> that's important enough for a 1.0 pullup?
>
> Personally, I'd rather not. We're already accumulating a lot of
> little proposed changes late in the game. This one doesn't seem that
> compelling, given that all we're preventing is a compile warning on
> some platforms.
It's a bit more than that, the report that led to this patch involved
using the xlc compiler on AIX, and it was a compiler *error*. xlc
also has the "volatile sig_atomic_t" problem for which I don't have a
ready solution, other than suggesting that those that encounter it
delete volatile from the code. I suspect neither problem affects gcc
on AIX.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r8343 - trunk/subversion/libsvn_ra_svn
Posted by kf...@collab.net.
Greg Hudson <gh...@MIT.EDU> writes:
> > * subversion/libsvn_ra_svn/cram.c (compute_digest): Use an unsigned
> > char array for the secret. Patch from Philip Martin.
>
> The bitwise operations performed on secret are all xors, so there should
> be no semantic problem with them being (possibly) signed char. So, it's
> mainly an issue of warnings with pickier compilers. Do people think
> that's important enough for a 1.0 pullup?
Personally, I'd rather not. We're already accumulating a lot of
little proposed changes late in the game. This one doesn't seem that
compelling, given that all we're preventing is a compile warning on
some platforms.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r8343 - trunk/subversion/libsvn_ra_svn
Posted by Ben Reser <be...@reser.org>.
On Fri, Jan 16, 2004 at 01:30:29PM -0500, Greg Hudson wrote:
> The bitwise operations performed on secret are all xors, so there should
> be no semantic problem with them being (possibly) signed char. So, it's
> mainly an issue of warnings with pickier compilers. Do people think
> that's important enough for a 1.0 pullup?
I thought we treated warnings as bugs? If so it's a bug and if it
doesn't hurt anything might as well do it.
--
Ben Reser <be...@reser.org>
http://ben.reser.org
"Conscience is the inner voice which warns us somebody may be looking."
- H.L. Mencken
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r8343 - trunk/subversion/libsvn_ra_svn
Posted by Branko Čibej <br...@xbc.nu>.
Greg Hudson wrote:
>On Fri, 2004-01-16 at 13:20, ghudson@tigris.org wrote:
>
>
>>* subversion/libsvn_ra_svn/cram.c (compute_digest): Use an unsigned
>> char array for the secret. Patch from Philip Martin.
>>
>>
>
>The bitwise operations performed on secret are all xors, so there should
>be no semantic problem with them being (possibly) signed char.
>
But there can be a difference on one's complement or sign+magnitude
machines. They're about as rare as the ones where all-bits-zero is not
the null pointer, but if we're being pedaintic, the standard does say
that "unsigned char" is the correct representation for raw memory, not
plain char.
> So, it's
>mainly an issue of warnings with pickier compilers. Do people think
>that's important enough for a 1.0 pullup?
>
>
No.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org