Bug#493637: insserv_1.12.0-1(m68k/unstable): alignment issues.
On Mon, Aug 04, 2008 at 10:09:07PM +0200, Petter Reinholdtsen wrote:
[Dr. Werner Fink]
Beside speed there is no problem to drop the alignment.
Nevertheless it is very strange that the struct re_pattern_buffer,
the real type of regex_t, and found in /usr/include/regex.h is not
aligned within power of 2.
There are some more alignment I've done for 1.12.0 (speed over
size). Maybe this will show similar problems.
It seem my guess about this being a speed optimization was correct.
As I said: using alignment for optimization is not usually a good idea,
unless you _really_ know what you're doing, and even then only in
architecture-specific code. The effect of manually specifying alignment
like that will have wildly different results from one architecture to
another; it's usually safe to trust the compiler to know what will be
fastest.
But the fact that there might be more of them make me hope for a patch
from someone with access to a m68k machine, to make sure I do not
upload a new version fixing only part of the problems, and thus having
to do more than one upload to fix the issue.
Why have them in the first place? If you just remove them all, that will
surely fix the issue. I seriously doubt this benefits performance on
_all_ architectures where it is currently enabled, anyway.
If, however, there is indeed a measureable performance difference on
some architecture(s), then you could make the code look similar to this:
struct foo {
int foo;
int bar;
#ifdef ARCHITECTURE_ON_WHICH_WE_TESTED
} attribute((aligned(whatever)));
#else
}
#endif
with ARCHITECTURE_ON_WHICH_WE_TESTED being defined on an architecture
where you actually did measure performance differences.
[Wouter Verhelst]
If you explicitly specify alignment like that, you disable any
automatic stuff the compiler might otherwise want to perform.
Sure. I guess I wonder about why the alignment requirement isn't
fulfilled when asked for the aligment of the sizeof(regex_t). Can
someone with m68k test and provide the answer for these questions:
What is the value of sizeof(regex_t) on m68k? What is the alignment
calculated for the struct?
sizeof(regex_t) is 30 on m68k, so I guess that's also what the
calculated alignment is.
The fact that the code works on, e.g., sparc probably means the relevant
structures there just happen to be aligned on the correct boundaries.
--
Lo-lan-do Home is where you have to wash the dishes.
-- #debian-devel, Freenode, 2004-09-22
--
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#493637: insserv_1.12.0-1(m68k/unstable): alignment issues.
[Wouter Verhelst] it's usually safe to trust the compiler to know what will be fastest. I agree. Why have them in the first place? If you just remove them all, that will surely fix the issue. To me it is more a question of how to be sure all of them are removed in a way that get insserv working. For that, I would like someone with m68k access to come up with a patch, thus making sure the patch actually solve the problem on m68k. Because of this, I hope you or someone else with m68k machines can provide a tested patch. What is the value of sizeof(regex_t) on m68k? What is the alignment calculated for the struct? sizeof(regex_t) is 30 on m68k, so I guess that's also what the calculated alignment is. Now I am confused. The error complained that the alignment wasn't on 2-byte boundaries, if I understood it correctly. 30 is divisable by 2, and thus aligning on 30 byte boundaries should also be on 2-byte boundaries. How did it end up not being on 2-byte boundaries? What did I misunderstand? Happy hacking, -- Petter Reinholdtsen -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#493637: insserv_1.12.0-1(m68k/unstable): alignment issues.
On Tue, Aug 05, 2008 at 04:14:07PM +0200, Petter Reinholdtsen wrote: [Wouter Verhelst] it's usually safe to trust the compiler to know what will be fastest. I agree. Why have them in the first place? If you just remove them all, that will surely fix the issue. To me it is more a question of how to be sure all of them are removed in a way that get insserv working. For that, I would like someone with m68k access to come up with a patch, thus making sure the patch actually solve the problem on m68k. Because of this, I hope you or someone else with m68k machines can provide a tested patch. 'grep -r attribute *' in the source directory only comes up with four lines. One of them is a macro that makes things align on sizeof(something*), which isn't likely to be a problem in the real world (I've yet to see an architecture where alignment on the size of a pointer is going to be problematic), and two of the others align on sizeof(char*). The last two are the problematic ones in this bugreport... But, sure, I can test-build on an m68k machine. I'll let you know what the results are. What is the value of sizeof(regex_t) on m68k? What is the alignment calculated for the struct? sizeof(regex_t) is 30 on m68k, so I guess that's also what the calculated alignment is. Now I am confused. The error complained that the alignment wasn't on 2-byte boundaries, if I understood it correctly. Actually, it complained about it not being aligned on a power-of-two boundary. 30 isn't a power of two. Your confusion is a result of my confusion in this bugreport, where I confused the fact that m68k, occasionally, also requires alignment on a 2-byte boundary on the frame pointer -- but that is so entirely and utterly different from what we're talking about here that it isn't even funny. Sorry about that. In reality, m68k requires access aligned to the length of a data type; i.e., 1-byte for char, 2-byte for short, and 4-byte for int/float/pointer (though it gets less clear after that). In order to be able to enforce this requirement, the ABI introduces a requirement that structs be aligned to the size of their largest member, and gcc turns that into a power-by-two alignment requirement (if I read the ABI docs correctly). In any case, aligning on 30 bytes will surely break the 4-byte requirement of pointers and ints, which regex_t does contain. 30 is divisable by 2, and thus aligning on 30 byte boundaries should also be on 2-byte boundaries. Indeed. But that's not the problem. -- Lo-lan-do Home is where you have to wash the dishes. -- #debian-devel, Freenode, 2004-09-22 -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#493637: insserv_1.12.0-1(m68k/unstable): alignment issues.
On m68k, some bits of memory must be aligned on a 2-byte boundary. The above makes that impossible. I notice all the other architectures handled this code. This make me wonder if this could be seen as a bug in the compiler on m68k. Why does it not calculate the required 2-byte alignment if it need the data to be aligned on 2-byte boundaries? Note, I am not against changing the code of insserv to get it working on m68k, but I want to understand why the correct fix is in that code, and not in the compiler on m68k, when the code work on all other architectures. What is the value of sizeof(regex_t) on m68k? What is the alignment calculated for the struct? Happy hacking, -- Petter Reinholdtsen -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#493637: insserv_1.12.0-1(m68k/unstable): alignment issues.
On Mon, Aug 04, 2008 at 11:03:34AM +0200, Petter Reinholdtsen wrote: On m68k, some bits of memory must be aligned on a 2-byte boundary. The above makes that impossible. I notice all the other architectures handled this code. This make me wonder if this could be seen as a bug in the compiler on m68k. Why does it not calculate the required 2-byte alignment if it need the data to be aligned on 2-byte boundaries? If you explicitly specify alignment like that, you disable any automatic stuff the compiler might otherwise want to perform. If you do not specify an explicit alignment requirement, the compiler will happily align everything on 2-byte boundaries when it needs to, no problem. The bug is not in the compiler. Note, I am not against changing the code of insserv to get it working on m68k, but I want to understand why the correct fix is in that code, and not in the compiler on m68k, when the code work on all other architectures. It is definately not a bug in the compiler to attempt to follow the programmer's directives and to barf out if that is impossible... What is the value of sizeof(regex_t) on m68k? What is the alignment calculated for the struct? Happy hacking, -- Petter Reinholdtsen -- Lo-lan-do Home is where you have to wash the dishes. -- #debian-devel, Freenode, 2004-09-22 -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#493637: insserv_1.12.0-1(m68k/unstable): alignment issues.
On Sun, Aug 03, 2008 at 08:55:23PM +0200, Petter Reinholdtsen wrote: If the issue is that you want to use these structs in a protocol of some sort or other, then it's probably best to use attribute(__packed__), which has the same effect as the above, but with the added benefit of the compiler generating some stubs in code so that it can be safely accessed (if with a performance penalty) from 'regular' code. I have sent an email to upstream asking wy it is done like this. I suspect it is to optimize for speed, If that is the case, then doing it like this is a *very* bad idea. The result on performance of an explicit alignment will be completely different across different architectures; while it may improve speed on some architecture (presumably the one the original developer tested it on), there's no saying what result it may have on others without actually reading the architecture's documentation and/or testing code there. Alignment really is an example of things you shouldn't be playing with unless you *really* know what you're doing, and then only inside an #ifdef __mc68000__ or #ifdef __SPARC__ or something similar. as there is no protocol involved, but am not sure. Does it still work on m68k if you remove the alignment attribute? I haven't tried, but I can't imagine it not working like that. -- Lo-lan-do Home is where you have to wash the dishes. -- #debian-devel, Freenode, 2004-09-22 -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#493637: insserv_1.12.0-1(m68k/unstable): alignment issues.
I got this reply from upstream regarding this issue: [Dr. Werner Fink] Beside speed there is no problem to drop the alignment. Nevertheless it is very strange that the struct re_pattern_buffer, the real type of regex_t, and found in /usr/include/regex.h is not aligned within power of 2. There are some more alignment I've done for 1.12.0 (speed over size). Maybe this will show similar problems. It seem my guess about this being a speed optimization was correct. But the fact that there might be more of them make me hope for a patch from someone with access to a m68k machine, to make sure I do not upload a new version fixing only part of the problems, and thus having to do more than one upload to fix the issue. [Wouter Verhelst] If you explicitly specify alignment like that, you disable any automatic stuff the compiler might otherwise want to perform. Sure. I guess I wonder about why the alignment requirement isn't fulfilled when asked for the aligment of the sizeof(regex_t). Can someone with m68k test and provide the answer for these questions: What is the value of sizeof(regex_t) on m68k? What is the alignment calculated for the struct? It is definately not a bug in the compiler to attempt to follow the programmer's directives and to barf out if that is impossible... I am trying to understand why it is impossible. Can you explain it to me? Happy hacking, -- Petter Reinholdtsen -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#493637: insserv_1.12.0-1(m68k/unstable): alignment issues.
Package: insserv
Version: 1.12.0-1
Severity: important
There was an error while trying to autobuild your package:
Automatic build of insserv_1.12.0-1 on arrakis by sbuild/m68k 98
Build started at 20080801-0340
[...]
** Using build dependencies supplied by package:
Build-Depends: debhelper (= 4.0.0), po-debconf, dpatch
[...]
dh_testdir
# Add here commands to compile the package.
/usr/bin/make COPTS=-g -O2
make[1]: Entering directory `/build/buildd/insserv-1.12.0'
gcc -W -Wall -g -O2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64
-DINITDIR=\/etc/init.d\ -DINSCONF=\/etc/insserv.conf\ -pipe
-falign-loops=0 -c insserv.c
In file included from insserv.c:41:
listing.h: In function 'prefetch':
listing.h:58: warning: unused parameter 'x'
insserv.c: At top level:
insserv.c:150: error: requested alignment is not a power of 2
insserv.c:155: error: requested alignment is not a power of 2
make[1]: *** [insserv.o] Error 1
make[1]: Leaving directory `/build/buildd/insserv-1.12.0'
make: *** [build-stamp] Error 2
dpkg-buildpackage: failure: debian/rules build gave error exit status 2
insserv.c contains the following:
/* Search results points here */
typedef struct reg_struct {
regex_t prov;
regex_t req_start;
regex_t req_stop;
regex_t shl_start;
regex_t shl_stop;
regex_t start_bf;
regex_t stop_af;
regex_t def_start;
regex_t def_stop;
regex_t desc;
} attribute((aligned(sizeof(regex_t reg_t;
With the final line of the above snippet being line 150.
On m68k, some bits of memory must be aligned on a 2-byte boundary. The
above makes that impossible.
If the issue is that you want to use these structs in a protocol of some
sort or other, then it's probably best to use attribute(__packed__),
which has the same effect as the above, but with the added benefit of
the compiler generating some stubs in code so that it can be safely
accessed (if with a performance penalty) from 'regular' code.
A full build log can be found at:
http://buildd.debian.org/build.php?arch=m68kpkg=insservver=1.12.0-1
--
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#493637: insserv_1.12.0-1(m68k/unstable): alignment issues.
If the issue is that you want to use these structs in a protocol of some sort or other, then it's probably best to use attribute(__packed__), which has the same effect as the above, but with the added benefit of the compiler generating some stubs in code so that it can be safely accessed (if with a performance penalty) from 'regular' code. I have sent an email to upstream asking wy it is done like this. I suspect it is to optimize for speed, as there is no protocol involved, but am not sure. Does it still work on m68k if you remove the alignment attribute? Happy hacking, -- Petter Reinholdtsen -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
