Re: STDCXX-1066 [was: Re: STDCXX forks]
On Mon, Sep 24, 2012 at 8:21 AM, Liviu Nicoara nikko...@hates.ms wrote: In the light of your inability to answer the simplest questions about the correctness and usefulness of this patch, I propose we strike the patch in its entirety. Let me make something very clear to you: what I am doing here is a courtesy to the stdcxx project. There is no requirement in my job description to waste hours arguing with you in pointless squabbles over email. Nor is there a requirement in the APL V2.0 which would somehow compel us to redistribute our source code changes. We could and should re-work the instances in the library where we might use mutex and condition objects that are misaligned wrt the mentioned kernel update. Yeah, why don't you go ahead and do that. Just like you fixed stdcxx-1056. --Stefan -- Stefan Teleman KDE e.V. stefan.tele...@gmail.com
RE: STDCXX-1066 [was: Re: STDCXX forks]
Comments below...
-Original Message-
From: Stefan Teleman
Sent: Monday, September 24, 2012 5:46 AM
Subject: Re: STDCXX-1066 [was: Re: STDCXX forks]
On Mon, Sep 24, 2012 at 8:21 AM, Liviu Nicoara nikko...@hates.ms
wrote:
In the light of your inability to answer the simplest questions about
the correctness and usefulness of this patch, I propose we strike the
patch in its entirety.
Let me make something very clear to you: what I am doing here is a
courtesy to the stdcxx project.
Thank you for your contribution. There is a good chance that without your
activity on this project over the last few months that it would be in the attic.
There is no requirement in my job
description to waste hours arguing with you in pointless squabbles
over email. Nor is there a requirement in the APL V2.0 which would
somehow compel us to redistribute our source code changes.
The problem here is that code changes are expected to pass review. They must
follow established conventions, solve the problem in a reasonable and portable
way, provide a test case (where one doesn't already exist), as well as make
sense to everyone who is working with the product.
Several of the changes included don't make sense to the rest of us. Unless we
manage to figure them out on our own or you manage to explain them to us, then
the changes should probably be excluded from the code.
For example, the changes to src/exception.cpp don't make sense to me. Here is
the relevant diff...
+#if defined(_RWSTD_STRICT_SPARCV8_MUTEX_ALIGNMENT)
+# pragma pack(8)
+# pragma align 8(__rw_aligned_buffer)
+#endif
+
// facet must never be destroyed
static __rw_aligned_buffer_Msgs msgs;
It seems that we trying to give 8-byte alignment to a class of type
__rw_aligned_buffer. The point of __rw_aligned_buffer is to give us a block of
properly aligned data, and if it isn't aligned, then it is broken. So, why are
we using a pragma for alignment here (and potentially in other places) when it
seems that there is a bug in __rw_aligned_buffer that we should be addressing?
Of course, if this is a problem with aligned buffer, we need a testcase.
Another example are the changes to src/ctype.cpp
+#if defined(_RWSTD_STRICT_SPARCV8_MUTEX_ALIGNMENT)
+# pragma pack(8)
+# pragma align 8(f)
+# pragma align 8(pf)
+#endif
static union {
+#if defined(_RWSTD_STRICT_SPARCV8_MUTEX_ALIGNMENT)
+unsigned long long align_;
+unsigned char data_ [sizeof (_STD::ctypechar)];
+#else
void *align_;
char data_ [sizeof (_STD::ctypechar)];
+#endif
} f;
static __rw_facet* const pf =
new (f) _STD::ctypechar(__rw_classic_tab, false, ref);
pfacet = pf;
+#if defined(_RWSTD_STRICT_SPARCV8_MUTEX_ALIGNMENT)
+# pragma pack(0)
+#endif
This change seems to give alignment to `f' in two different ways, and it seems
to be applying alignment to the pointer (the issue Liviu has brought up). It
seems like the simpler fix would be to unconditionally add union members to `f'
so that we get the proper alignment (much like we try to do in
__rw_aligned_buffer), or an even better solution would be to use
__rw_aligned_buffer.
We could and should re-work the instances in the library where
we might use mutex and condition objects that are misaligned wrt the
mentioned kernel update.
Yeah, why don't you go ahead and do that. Just like you fixed stdcxx-
1056.
There is no need for comments like this.
Stefan, if you are feeling singled out and attacked because your patches aren't
accepted without question, it might be a good idea to go back and look through
the mailing list archives. We have _all_ been in this position. Nearly every
time I posted a patch for the first year working on stdcxx was like this, and
I've been doing C++ software for 15 years.
The company where I work currently has a review process that is just as
rigorous as what you're seeing with stdcxx now. The process isn't there to
attack anyone personally, but to ensure the quality and maintainability of the
library for years to come.
Travis
Re: STDCXX-1066 [was: Re: STDCXX forks]
On 09/16/12 12:03, Stefan Teleman wrote: On Sat, Sep 15, 2012 at 5:47 PM, Liviu Nicoara nikko...@hates.ms wrote: I merely wanted to point out that restoring the default packing is not the same as restoring the packing to the previous value in effect. Given this, I thought about an alternative way of forcing this alignment, e.g., via a union, aligned on an appropriate type. I see an advantage here in that most of the changes would occur where we define the 'primitive' mutex and condition wrappers, saving a few pre-processor conditionals and pragmas along the way. I understood what you were saying. I just don't understand under what _sane_ circumstances a program would #include a system library header file with a previously set packing to something other than default. Or would expect a non-default packing to work during a function call to a sytem library. That's an insane configuration on any operating system that I know of, not just on SPARCV8. I can't think of a valid scenario, either. I guess the point is moot. A few more questions, if you will, as I am going through the changes: 1. I see similarities with 1040, should/would you close that one? 2. The issue only exists in MT builds, should there be a guard in configs? 3. The align reference docs talk only about aligning variables, not types. Is that different on SPARC? 4. I see rw/_mutex.h has alignment pragmas for both __rw_mutex_base class and its mutex member; same for __rw_static_mutex and its static member, etc. How does that work? 5. Why is __rw_guard aligned explicitly? I see it only contains a pointer to a mutex object. 6. The docs mention that the pragma must use the mangled variables names but I don't see that in the patch. Thanks! Liviu
Re: STDCXX-1066 [was: Re: STDCXX forks]
On Sun, Sep 23, 2012 at 3:26 PM, Liviu Nicoara nikko...@hates.ms wrote: To be honest it's quite bizarre that you cannot share that with us. Is it really a trade secret? How can that be the case if Oracle customers are also required to perform the same alignment, perhaps using the same techniques like you showed in the patch? That's the problem. I don't know what is and what is not a trade secret, or copyrighted information, or unauthorized intellectual property disclosure anymore. I think it's in the eye of the beholder. At Sun it was very clear. Believe it or not, I had to get written approval from the Legal Counsel's Office in order to be able to share these patches. And that in spite of the fact that these patches are published, and have already been published for *years*. IANAL and I don't want to play one on TV. ;-) --Stefan -- Stefan Teleman KDE e.V. stefan.tele...@gmail.com
Re: STDCXX-1066 [was: Re: STDCXX forks]
On 9/23/12 3:48 PM, Stefan Teleman wrote: On Sun, Sep 23, 2012 at 3:26 PM, Liviu Nicoara nikko...@hates.ms wrote: To be honest it's quite bizarre that you cannot share that with us. Is it really a trade secret? How can that be the case if Oracle customers are also required to perform the same alignment, perhaps using the same techniques like you showed in the patch? That's the problem. I don't know what is and what is not a trade secret, or copyrighted information, or unauthorized intellectual property disclosure anymore. I think it's in the eye of the beholder. At Sun it was very clear. Believe it or not, I had to get written approval from the Legal Counsel's Office in order to be able to share these patches. And that in spite of the fact that these patches are published, and have already been published for *years*. That clarifies things. Thanks. Liviu
Re: STDCXX-1066 [was: Re: STDCXX forks]
On Sun, Sep 23, 2012 at 5:23 PM, Stefan Teleman stefan.tele...@gmail.com wrote: The second URL says this: QUOTE Due to a change in the implementation of the userland mutexes introduced by CR 6296770 in KU 137111-01, objects of type mutex_t and pthread_mutex_t must start at 8-byte aligned addresses. If this requirement is not satisfied, all non-compliant applications on Solaris/SPARC may fail with the signal SEGV with a callstack similar to the following one or with similar callstacks containing the function mutex_trylock_process. \*_atomic_cas_64(0x141f2c, 0x0, 0xff00, 0x1651, 0xff00, 0x466d90) set_lock_byte64(0x0, 0x1651, 0xff00, 0x0, 0xfec82a00, 0x0) fast_process_lock(0x141f24, 0x0, 0x1, 0x1, 0x0, 0xfeae5780) /QUOTE Here's a link to an official datatype alignment table for SPARCV8: http://docs.oracle.com/cd/E19205-01/819-5267/bkbkl/index.html The interesting table is: Table B–2 Storage Sizes and Default Alignments in Bytes --Stefan -- Stefan Teleman KDE e.V. stefan.tele...@gmail.com
Re: STDCXX-1066 [was: Re: STDCXX forks]
On 9/23/12 5:50 PM, Stefan Teleman wrote: On Sun, Sep 23, 2012 at 5:23 PM, Stefan Teleman stefan.tele...@gmail.com wrote: The second URL says this: QUOTE Due to a change in the implementation of the userland mutexes introduced by CR 6296770 in KU 137111-01, objects of type mutex_t and pthread_mutex_t must start at 8-byte aligned addresses. If this requirement is not satisfied, all non-compliant applications on Solaris/SPARC may fail with the signal SEGV with a callstack similar to the following one or with similar callstacks containing the function mutex_trylock_process. \*_atomic_cas_64(0x141f2c, 0x0, 0xff00, 0x1651, 0xff00, 0x466d90) set_lock_byte64(0x0, 0x1651, 0xff00, 0x0, 0xfec82a00, 0x0) fast_process_lock(0x141f24, 0x0, 0x1, 0x1, 0x0, 0xfeae5780) /QUOTE Here's a link to an official datatype alignment table for SPARCV8: http://docs.oracle.com/cd/E19205-01/819-5267/bkbkl/index.html The interesting table is: Table B–2 Storage Sizes and Default Alignments in Bytes I see nothing really outstanding here. What is it that I should pay attention to? Thanks, Liviu
Re: STDCXX-1066 [was: Re: STDCXX forks]
On Sun, Sep 23, 2012 at 7:25 PM, Liviu Nicoara nikko...@hates.ms wrote: I am not asking for any other implementation and I am not looking to change anything. I wish you could explain it to us, but in the absence of trade secret details I will take an explanation for the questions above. Sorry, no. This will not be another replay of the stdcxx-1056 email discussion, which was a three week waste of my time. The patch is provided AS IS. No further testing and no further comments. I do have more important things to do. -- Stefan Teleman KDE e.V. stefan.tele...@gmail.com
Re: STDCXX-1066 [was: Re: STDCXX forks]
On 9/15/12 2:57 PM, Stefan Teleman wrote:
On Sat, Sep 15, 2012 at 1:02 PM, Liviu Nicoara nikko...@hates.ms wrote:
I have read through the patches attached to the incident, then I briefly
read about the SunPro pragma align and pack. Two questions:
1. AFAICT, the use of the packing pragma may interfere with a user's setting
of the same value. I.e., a user sets the packing in their sources and then,
directly or not, includes an STDCXX header. It seems to me that in such a
situation, our setting of the packing value would interfere with the rest of
the user's translation unit, since there is no way to `restore' the previous
packing value.
Something along the lines of:
// user source file
#pragma pack (X) // X != 8
#include iostream
struct UserDef
{
// different alignment than X ?
// ...
};
Is my understanding correct?
Yes, you are indeed correct, Sir. :-)
However, for every #pragma pack(8) there should be a corresponding
subsequent #pragma pack(0). If there isn't, that's a patch bug.
#pragma pack(0) restores the default alignment. So, there should be
(there *must* be) no silent packing side-effects in user programs.
Yes, but it restores the default packing, not an arbitrary one, potentially set
by the user prior to including our headers. Say, the user sets 2, the default is
4 and we set 8. When we set it to default it goes back to 4, instead of the
expected 2. Did I get this right?
2. The patches are against 4.2.1, but the change would be binary
incompatible with the already released 4.2.1 branch. Do you plan to have
this fix in 4.3.x?
Yes, definitely for 4.2.x and 4.3.x. I sent them for 4.2.1 just
because that's what I have handy.
I see now. I tried to apply them to 4.2.x and some chunks failed, and I thought
I was not applying them where they were intended to go.
Thanks!
Liviu
Re: STDCXX-1066 [was: Re: STDCXX forks]
On 9/15/12 5:19 PM, Stefan Teleman wrote: On Sat, Sep 15, 2012 at 4:57 PM, Liviu Nicoara nikko...@hates.ms wrote: Yes, but it restores the default packing, not an arbitrary one, potentially set by the user prior to including our headers. Say, the user sets 2, the default is 4 and we set 8. When we set it to default it goes back to 4, instead of the expected 2. Did I get this right? This is true, but leaving some arbirary pragma pack(N) (for N != 0) in effect for the duration of a program, and expecting it to work sounds like a very defective programming approach to me. It will certainly not work on SPARC at all. if the program needs to pack something in a certain specific way, it need to do so for that specific something only. Otherwise the side-effects of globally setting a non-default packing will destroy the program anyway. I merely wanted to point out that restoring the default packing is not the same as restoring the packing to the previous value in effect. Given this, I thought about an alternative way of forcing this alignment, e.g., via a union, aligned on an appropriate type. I see an advantage here in that most of the changes would occur where we define the 'primitive' mutex and condition wrappers, saving a few pre-processor conditionals and pragmas along the way. What do you think? Liviu
