[PATCH] Fix unsigned character bugs in ginac-1.5.8
Hi There are a couple of bugs that stop ginac from working on machines whose characters are unsigned by default (where a conversion to int goves a value 0-255). It's the classic "char c = getchar(); if (c == EOF)" with result that it doesn't detect EOF on unsigned machines, or exits prematurely on signed machines if it reads a character 255. The fixes are to replace "char" with "int" so that all 257 possible return values are correctly handled. The attached patch for ginac-1.4.3 still applies to 1.5.8 Cheers M
Hello, On Sat, Apr 02, 2011 at 12:08:24PM +0200, Martin Guy wrote:
This patch fixes a bug on machines where char is unsigned by default, by extending the type of clifford_max_label to include all 257 possible return values.
I'm sorry to confuse you, but your patch is wrong. It just breaks ABI for nothing good at all. In practice there's no need for gamma matrices with 128 representation labels (otherwise GiNaC would have problems on machines where char is *signed* by default). I guess we should fix the documentation (it claims that representation labels from 0 to 255 are supported, which is definitely wrong). Best regards, Alexei
On 2 April 2011 14:06, Alexei Sheplyakov <alexei.sheplyakov@gmail.com> wrote:
On Sat, Apr 02, 2011 at 12:08:24PM +0200, Martin Guy wrote:
This patch fixes a bug on machines where char is unsigned by default, by extending the type of clifford_max_label to include all 257 possiblet return values.
I'm sorry to confuse you, but your patch is wrong. It just breaks ABI for nothing good at all. In practice there's no need for gamma matrices with 128 representation labels (otherwise GiNaC would have problems on machines where char is *signed* by default). I guess we should fix the documentation (it claims that representation labels from 0 to 255 are supported, which is definitely wrong).
Eh? On unsigned char systems it currently fails to detect an error condition, but continues blindly, wihle users who have spotted the defect are forced to detect errors using "if (x & 255 == EOF & 255), which is crazy. In practice, chars are passed and returned as ints, so nothing changes in the function call/return values (which is what I thought an ABI was). Since users presumably include clifford.h before calling the function, at most they will get a compiler warning about an integer being stored in a character if they have done that, which also correctly pinpoints the error in their code. Anyway M
Hi! On 04/02/2011 02:06 PM, Alexei Sheplyakov wrote:
On Sat, Apr 02, 2011 at 12:08:24PM +0200, Martin Guy wrote:
This patch fixes a bug on machines where char is unsigned by default, by extending the type of clifford_max_label to include all 257 possible return values.
I'm sorry to confuse you, but your patch is wrong. It just breaks ABI for nothing good at all. In practice there's no need for gamma matrices with 128 representation labels (otherwise GiNaC would have problems on machines where char is *signed* by default). I guess we should fix the documentation (it claims that representation labels from 0 to 255 are supported, which is definitely wrong).
Alexei, does it really break the ABI? Whether we return an int here or a char won't change library symbol names and since chars are returned as ints, it won't change anything binary-wise. There are cases where clifford_max_label returns -1, so this is indeed broken. And this doesn't seem to have anything to do with having 255 different representation labels (which would be crazy, I agree.) So, I don't see a problem with Martin's patch. Did I miss anything? -richy. -- Richard B. Kreckel <http://www.ginac.de/~kreckel/>
Hello, On Wed, Apr 06, 2011 at 09:33:50AM +0200, Richard B. Kreckel wrote:
Alexei, does it really break the ABI? Whether we return an int here or a char won't change library symbol names and since chars are returned as ints, it won't change anything binary-wise.
Changing the return type might break the ABI, and I think it's better to be safe than sorry.
There are cases where clifford_max_label returns -1, so this is indeed broken.
clifford_max_label returns some magic number (-1 aka 255) to indicate that the expression does not contain any gamma matrices. It's perfectly fine to do so as long as no valid representation label coincides with that magic number. Let's forget about computer algebra for a second and consider the following (C) code: #include <stdlib.h> /* size_t */ int max_val_s(const unsigned int* data, size_t sz) { size_t i = 0; unsigned int val = 0; if ((!data) || (!sz)) return -1; /* -1 is just a magic number which means "the input is empty */ for (; i < sz; ++i) { if (val < data[i]) val = data[i]; } return val; } The max_val_s is perfectly fine as long as the input data does not contain values greater than INT_MAX (usually 2^31 - 1). unsigned int max_val_u(const unsigned int* data, size_t sz) { size_t i = 0; unsigned int val = 0; if ((!data) || (!sz)) return -1; /* -1 is just a magic number which means "the input is empty */ for (; i < sz; ++i) { if (val < data[i]) val = data[i]; } return val; } The max_val_u is fine, too, as long as the input values are less than UINT_MAX. The above statements still hold if one replaces int with char (INT_MAX with CHAR_MAX, and UINT_MAX with UCHAR_MAX, respectively). Thus, clifford_max_label is OK as long as representation labels are within [0, 127] range (no matter if char is signed or not).
So, I don't see a problem with Martin's patch. Did I miss anything?
Apart from a possible ABI change the patch is harmless. On the other hand, it's useless (see the explanation above), so I don't see any reason to apply it (as in "if it ain't broke, don't fix it"). Best regards, Alexei
participants (3)
-
Alexei Sheplyakov
-
Martin Guy
-
Richard B. Kreckel