Re: STDCXX-1071 numpunct facet defect
I thought I replied but I see no trace of my post: On 09/27/12 20:27, Martin Sebor wrote: On 09/27/2012 06:41 AM, Liviu Nicoara wrote: On 09/26/12 20:12, Liviu Nicoara wrote: I have created STDCXX-1071 and linked to STDCXX-1056. [...] I am open to all questions, the more the better. Most of my opinions have been expressed earlier, but please ask if you want to know more. I am attaching here the proposed (4.3.x) patch and the timings results (after re-verifying the correctness of the timing program and the results). The 4.2.x patch, the 4.3.x patch, the test program and the results file are also attached to the incident. The patch isn't binary compatible. We can't remove data members in a minor release. We could only do it in a major release. I posted two patches, one for 4.3.x and one for 4.2.x (the latter is binary compatible). My understanding was that minor version revisions may break binary compatibility. Liviu
STDCXX-1072 SPARC V8 mutex alignment requirements
I have created the above and linked it to the closed STDCXX-1066.
In short, my reading about this issue is that the kernel patch changed the
alignment of the userland mutex objects from a machine word to a double-word
boundary. No changes are required of the users who use such objects in their
programs unless users create mutex objects in buffers which may not be aligned
on a proper boundary. E.g., the following are safe:
mutex_t lock;
struct S {
char misalign;
mutex_t lock;
};
whereas the following is not:
union {
void* align;
char buf [sizeof mutex_t];
} u;
new (u) mutex_t;
because the alignment requirements for void pointer are less strict than for
mutex_t. A few places in the library use the latter for all sorts of static
objects (mostly local statics). I looked esp. for places where we build objects
that contain mutex sub-objects inside a union-aligned buffer:
struct S {
char c;
mutex_t m;
};
...
union {
void* align; // - incorrect
char buf [sizeof (S)];
} u;
new (u) S ();
The alignment must be changed to a value equal or greater than the mutex
alignment requirements.
IMO, the patch I attached does not break binary compatibility. It uses a one
size fits all long double for alignment -- like we use in rw/_mutex.h -- but in
doing so dispenses with all sorts of preprocessor conditionals.
I still don't have access to a SPARC machine. Any feed-back and/or SPARC build
results are more than welcome!
Thanks!
Liviu
Index: src/messages.cpp
===
--- src/messages.cpp(revision 1390953)
+++ src/messages.cpp(working copy)
@@ -65,7 +65,7 @@ struct __rw_open_cat_data
{
nl_catd catd;
union {
-void *_C_align;
+long double _C_align;
char _C_data [sizeof (_STD::locale)];
} loc;
};
Index: src/locale_body.cpp
===
--- src/locale_body.cpp (revision 1390953)
+++ src/locale_body.cpp (working copy)
@@ -1025,7 +1025,7 @@ _C_manage (__rw_locale *plocale, const c
// the classic C locale is statically allocated
// and not destroyed during the lifetime of the process
static union {
-void* _C_align;
+long double _C_align;
char _C_buf [sizeof (__rw_locale)];
} classic_body;
Index: src/use_facet.h
===
--- src/use_facet.h (revision 1390953)
+++ src/use_facet.h (working copy)
@@ -64,7 +64,7 @@
else { \
/* construct an ordinary facet in static memory */ \
static union { \
- void *align_; \
+ long double align_; \
char data_ [sizeof (__rw_ ## fid ## _facet)]; \
} f;\
static __rw_facet* const pf = \
@@ -91,7 +91,7 @@
{ \
/* construct an ordinary facet in static memory */ \
static union { \
- void *align_; \
+ long double align_; \
char data_ [sizeof (__rw_ ## fid ## _facet)]; \
} f;\
static __rw_facet* const pf = \
Index: src/ctype.cpp
===
--- src/ctype.cpp (revision 1390953)
+++ src/ctype.cpp (working copy)
@@ -627,7 +627,7 @@ _RWSTD_EXPORT __rw_facet* __rw_ct_ctype
}
else {
static union {
-void *align_;
+long double align_;
char data_ [sizeof (_STD::ctypechar)];
} f;
static __rw_facet* const pf =
Index: src/locale_classic.cpp
===
--- src/locale_classic.cpp (revision 1390953)
+++ src/locale_classic.cpp (working copy)
@@ -39,7 +39,7 @@ _RWSTD_NAMESPACE (__rw) {
// static buffer for the classic C locale object
static union {
-void* _C_align;
+long double _C_align;
char _C_buf [sizeof (_STD::locale)];
} __rw_classic;
Re: STDCXX-1072 SPARC V8 mutex alignment requirements
On 09/28/12 08:29, Liviu Nicoara wrote: I have created the above and linked it to the closed STDCXX-1066. [...] IMO, the patch I attached does not break binary compatibility. Scratch this, I haven't thought it through. Thanks, Liviu
Re: STDCXX-1072 SPARC V8 mutex alignment requirements
On 09/28/12 08:45, Liviu Nicoara wrote: On 09/28/12 08:29, Liviu Nicoara wrote: I have created the above and linked it to the closed STDCXX-1066. [...] IMO, the patch I attached does not break binary compatibility. Scratch this, I haven't thought it through. Actually, after more thought, I believe that the patch itself is not causing binary incompatibility. The library built on a kernel-patched system will be binary incompatible with a library built on a previous system, regardless of the patch I presented earlier, because new mutex alignment requirements will have changed the size and layout of library objects. Comments are welcome. Thanks, Liviu
RE: STDCXX-1071 numpunct facet defect
Only major versions can break binary. The versioning policy for stdcxx can be found here.. http://stdcxx.apache.org/versions.html Travis -Original Message- From: Liviu Nicoara [mailto:nikko...@hates.ms] Sent: Friday, September 28, 2012 3:52 AM To: dev@stdcxx.apache.org Subject: Re: STDCXX-1071 numpunct facet defect I thought I replied but I see no trace of my post: On 09/27/12 20:27, Martin Sebor wrote: On 09/27/2012 06:41 AM, Liviu Nicoara wrote: On 09/26/12 20:12, Liviu Nicoara wrote: I have created STDCXX-1071 and linked to STDCXX-1056. [...] I am open to all questions, the more the better. Most of my opinions have been expressed earlier, but please ask if you want to know more. I am attaching here the proposed (4.3.x) patch and the timings results (after re-verifying the correctness of the timing program and the results). The 4.2.x patch, the 4.3.x patch, the test program and the results file are also attached to the incident. The patch isn't binary compatible. We can't remove data members in a minor release. We could only do it in a major release. I posted two patches, one for 4.3.x and one for 4.2.x (the latter is binary compatible). My understanding was that minor version revisions may break binary compatibility. Liviu
Re: STDCXX-1071 numpunct facet defect
On 09/28/12 11:01, Travis Vitek wrote: Only major versions can break binary. The versioning policy for stdcxx can be found here.. http://stdcxx.apache.org/versions.html Thanks, that clarifies things. Liviu
RE: STDCXX-1072 SPARC V8 mutex alignment requirements
-Original Message- From: Liviu Nicoara Sent: Friday, September 28, 2012 5:29 AM In short, my reading about this issue is that the kernel patch changed the alignment of the userland mutex objects from a machine word to a double-word boundary. No changes are required of the users who use such objects in their programs unless users create mutex objects in buffers which may not be aligned on a proper boundary. Your reading of this is consistent with my previous understanding of the problem, so that is good. I still don't have access to a SPARC machine. Any feed-back and/or SPARC build results are more than welcome! I can provide build results for SPARCV9 if we want them, but I thought that the problem only came up on 32-bit SPARCV8 builds. The patch assumes the type `long double' exists on every platform. While I do believe that it is available everywhere, we have lots of conditional code guarding its use in the library now. If we are going to use `long double' in this context, we should guard it with _RWSTD_NO_LONG_DOUBLE. I think an even cleaner solution is to switch to using __rw_aligned_buffer instead. It gives us a single point of failure for alignment issues like this, and it makes the code self-documenting and easier to read. As for your concerns about binary compatibility, I think that everything should be okay. We aren't changing the size of anything that is being passed around, we're just changing its alignment. I could be wrong, but I'm pretty sure that we're safe.
Re: STDCXX-1072 SPARC V8 mutex alignment requirements
The patch looks reasonable to me, except for the missing guard
for _RWSTD_NO_LONG_DOUBLE. For C++ 11 compilers, we might want
to replace the union with the alignas features. Of course, that
will require another configuration test and macro, and most
likely won't help the current Sun Studio compiler (unless it
already implements alignas). Although it's possible that the
compiler supports the GCC aligned attribute (we might want to
use it with Linux compilers versions that don't yet implement
C++ 11 alignas).
Martin
On 09/28/2012 06:29 AM, Liviu Nicoara wrote:
I have created the above and linked it to the closed STDCXX-1066.
In short, my reading about this issue is that the kernel patch changed
the alignment of the userland mutex objects from a machine word to a
double-word boundary. No changes are required of the users who use such
objects in their programs unless users create mutex objects in buffers
which may not be aligned on a proper boundary. E.g., the following are
safe:
mutex_t lock;
struct S {
char misalign;
mutex_t lock;
};
whereas the following is not:
union {
void* align;
char buf [sizeof mutex_t];
} u;
new (u) mutex_t;
because the alignment requirements for void pointer are less strict than
for mutex_t. A few places in the library use the latter for all sorts of
static objects (mostly local statics). I looked esp. for places where we
build objects that contain mutex sub-objects inside a union-aligned buffer:
struct S {
char c;
mutex_t m;
};
...
union {
void* align; // - incorrect
char buf [sizeof (S)];
} u;
new (u) S ();
The alignment must be changed to a value equal or greater than the mutex
alignment requirements.
IMO, the patch I attached does not break binary compatibility. It uses a
one size fits all long double for alignment -- like we use in
rw/_mutex.h -- but in doing so dispenses with all sorts of preprocessor
conditionals.
I still don't have access to a SPARC machine. Any feed-back and/or SPARC
build results are more than welcome!
Thanks!
Liviu
RE: STDCXX-1072 SPARC V8 mutex alignment requirements
Liviu, Sorry I didn't look until just now, but it appears that I could have re-opened STDCXX-1066. I only see the 'Reopen Issue' button for STDCXX issues, but it is most definitely there. Perhaps there is some sort of permission issue for you? Also, STDCXX-1066 appears to have been a duplicate of STDCXX-1040. Travis -Original Message- From: Liviu Nicoara Sent: Friday, September 28, 2012 5:29 AM I have created the above and linked it to the closed STDCXX-1066.
Re: STDCXX-1072 SPARC V8 mutex alignment requirements
On 09/28/2012 09:32 AM, Travis Vitek wrote: -Original Message- From: Liviu Nicoara Sent: Friday, September 28, 2012 5:29 AM In short, my reading about this issue is that the kernel patch changed the alignment of the userland mutex objects from a machine word to a double-word boundary. No changes are required of the users who use such objects in their programs unless users create mutex objects in buffers which may not be aligned on a proper boundary. Your reading of this is consistent with my previous understanding of the problem, so that is good. I still don't have access to a SPARC machine. Any feed-back and/or SPARC build results are more than welcome! I can provide build results for SPARCV9 if we want them, but I thought that the problem only came up on 32-bit SPARCV8 builds. The patch assumes the type `long double' exists on every platform. While I do believe that it is available everywhere, we have lots of conditional code guarding its use in the library now. If we are going to use `long double' in this context, we should guard it with _RWSTD_NO_LONG_DOUBLE. I think an even cleaner solution is to switch to using __rw_aligned_buffer instead. It gives us a single point of failure for alignment issues like this, and it makes the code self-documenting and easier to read. As for your concerns about binary compatibility, I think that everything should be okay. We aren't changing the size of anything that is being passed around, we're just changing its alignment. I could be wrong, but I'm pretty sure that we're safe. Alignment does matter for binary compatibility but since the patch increases it, and since the actual alignment for an object is guaranteed to be at least as strict as the requirement for its type, I think the change is both backward and forward compatible. The latter only for correctly written programs that aren't relying on the actual alignment being greater than required. Since this is an internal helper used only by stdcxx classes, I don't think we need to worry about such programs. Martin
Re: STDCXX-1072 SPARC V8 mutex alignment requirements
On 09/28/12 11:45, Travis Vitek wrote: Liviu, Sorry I didn't look until just now, but it appears that I could have re-opened STDCXX-1066. I only see the 'Reopen Issue' button for STDCXX issues, but it is most definitely there. Perhaps there is some sort of permission issue for you? It's ok, I think it's somewhat cleaner to create a new one and link it to the old one. Even if clean was not a concern, it was within Stefan's options to close the incident. I don't know. Also, STDCXX-1066 appears to have been a duplicate of STDCXX-1040. Yep, forgot about it, I am thinking about linking that one, too. Thanks.
Re: STDCXX-1072 SPARC V8 mutex alignment requirements
On 09/28/12 11:32, Travis Vitek wrote:
-Original Message-
From: Liviu Nicoara
Sent: Friday, September 28, 2012 5:29 AM
In short, my reading about this issue is that the kernel patch changed
the alignment of the userland mutex objects from a machine word to a
double-word boundary. No changes are required of the users who use such
objects in their programs unless users create mutex objects in buffers
which may not be aligned on a proper boundary.
Your reading of this is consistent with my previous understanding of the
problem, so that is good.
I still don't have access to a SPARC machine. Any feed-back and/or
SPARC build results are more than welcome!
I can provide build results for SPARCV9 if we want them, but I thought that the
problem only came up on 32-bit SPARCV8 builds.
I guess a -xarch=sparcv8 build will do. It is quite unlikely anybody has a real
32-bit SPARC hardware.
The patch assumes the type `long double' exists on every platform. While I do
believe that it is available everywhere, we have lots of conditional code
guarding its use in the library now. If we are going to use `long double' in
this context, we should guard it with _RWSTD_NO_LONG_DOUBLE. I think an even
cleaner solution is to switch to using __rw_aligned_buffer instead. It gives us
a single point of failure for alignment issues like this, and it makes the code
self-documenting and easier to read.
I took a cue from an alignment in the rw/_mutex.h code there. I'll do better.
As for your concerns about binary compatibility, I think that everything should
be okay. We aren't changing the size of anything that is being passed around,
we're just changing its alignment. I could be wrong, but I'm pretty sure that
we're safe.
The library object sizes and layouts will change with or without our patch.
Before the kernel patch our objects will have one layout and size and different
ones afterwards. E.g.:
struct locale {
int c;
pthread_mutex_t lock;
};
before kernel patching you'd have no padding between the members. That's what I
thought when firing that second post about compatibility.
Liviu
Re: STDCXX-1072 SPARC V8 mutex alignment requirements
On 09/28/2012 11:27 AM, Liviu Nicoara wrote: On 09/28/12 11:45, Travis Vitek wrote: Liviu, Sorry I didn't look until just now, but it appears that I could have re-opened STDCXX-1066. I only see the 'Reopen Issue' button for STDCXX issues, but it is most definitely there. Perhaps there is some sort of permission issue for you? It's ok, I think it's somewhat cleaner to create a new one and link it to the old one. Even if clean was not a concern, it was within Stefan's options to close the incident. I don't know. Jira issues are ours to decide what to do with, just like stdcxx code. If there's consensus that the bug should stay open we keep it open (or reopen it). Likewise, if we all agree to close it we close it. FWIW, if we're going to fix the problem noted in STDCXX-1066 (regardless of how) it would probably make sense to change the resolution of the issue from Won't Fix. Otherwise it could be confusing. One other comment: I would suggest choosing subjects for bug reports that reflect the problem rather than a fix for it or a rationale for it. For STDCXX-1066 I think something like Library mutex objects misaligned on SPARCV8 would better capture the problem than the current title. (It's also up to us to rename an issue if we find it more descriptive than the original.) Martin Also, STDCXX-1066 appears to have been a duplicate of STDCXX-1040. Yep, forgot about it, I am thinking about linking that one, too. Thanks.
Re: STDCXX-1072 SPARC V8 mutex alignment requirements
On 09/28/12 13:51, Martin Sebor wrote: [...] One other comment: I would suggest choosing subjects for bug reports that reflect the problem rather than a fix for it or a rationale for it. For STDCXX-1066 I think something like Library mutex objects misaligned on SPARCV8 would better capture the problem than the current title. (It's also up to us to rename an issue if we find it more descriptive than the original.) I can't do that myself. I looked at that and 1056 and there is no button for me to reopen, or to edit stuff which is not mine. Needless to say, I like my issues better. But I have no problems with the re-opening of the closed ones. Thanks, Liviu
Re: STDCXX-1072 SPARC V8 mutex alignment requirements
On 09/28/2012 11:55 AM, Liviu Nicoara wrote: On 09/28/12 13:51, Martin Sebor wrote: [...] One other comment: I would suggest choosing subjects for bug reports that reflect the problem rather than a fix for it or a rationale for it. For STDCXX-1066 I think something like Library mutex objects misaligned on SPARCV8 would better capture the problem than the current title. (It's also up to us to rename an issue if we find it more descriptive than the original.) I can't do that myself. I looked at that and 1056 and there is no button for me to reopen, or to edit stuff which is not mine. What you can do with an issue is determined by your Jira role and the Jira permission scheme for the project. Your roles are Committer and PMC member. Both Committers and PMC members (and pretty much all other roles) have the Resolve Issues Permission: Ability to resolve and reopen issues. This includes the ability to set a fix version. https://issues.apache.org/jira/plugins/servlet/project-config/STDCXX/permissions So you should be able to reopen it. The top of the STDCXX-1066 page should look similar to what's in the attachment screenshot. I.e., you should see a Reopen Issue button near the top, just to the right of the |Comment|Voters|Watch Issue|More Actions| tabs. If that's not what you see, we should figure out why. One reason would be if you were logged in with a different user id, e.g., if you had more than one account. I checked and you do seem to have two accounts. One with your old Rogue Wave address, and another @hates.ms. I couldn't tell which address was used on the People admin page (Jira just shows the user name), so I removed and re-added you with the current email address. Let me know if that didn't clear things up. Martin Needless to say, I like my issues better. But I have no problems with the re-opening of the closed ones. Thanks, Liviu
Re: STDCXX-1071 numpunct facet defect
On 9/28/12 11:01 AM, Travis Vitek wrote: Only major versions can break binary. The versioning policy for stdcxx can be found here.. http://stdcxx.apache.org/versions.html I have renamed the binary-incompatible patch as patch-5.0.x.diff. Thanks, Liviu Travis -Original Message- From: Liviu Nicoara [mailto:nikko...@hates.ms] Sent: Friday, September 28, 2012 3:52 AM To: dev@stdcxx.apache.org Subject: Re: STDCXX-1071 numpunct facet defect I thought I replied but I see no trace of my post: On 09/27/12 20:27, Martin Sebor wrote: On 09/27/2012 06:41 AM, Liviu Nicoara wrote: On 09/26/12 20:12, Liviu Nicoara wrote: I have created STDCXX-1071 and linked to STDCXX-1056. [...] I am open to all questions, the more the better. Most of my opinions have been expressed earlier, but please ask if you want to know more. I am attaching here the proposed (4.3.x) patch and the timings results (after re-verifying the correctness of the timing program and the results). The 4.2.x patch, the 4.3.x patch, the test program and the results file are also attached to the incident. The patch isn't binary compatible. We can't remove data members in a minor release. We could only do it in a major release. I posted two patches, one for 4.3.x and one for 4.2.x (the latter is binary compatible). My understanding was that minor version revisions may break binary compatibility. Liviu
