[Maria-developers] WebScaleSQL: Actually fix all the array bounds warnings
Hi Sergei and Alexey, during our recent discussion re WebScaleSQL patches we agreed to review $subj: https://github.com/webscalesql/webscalesql-5.6/commit/094187c4a20e8d0c02f172... All these warnings come from strings/decimal.c. I was able to reproduce them using gcc-4.8.1, cmake -DBUILD_CONFIG=mysql_release (basically needs -Wall -O3). I simplified this function to the following: gcc -Wall -O3 -c dec.c -o dec.o static const int powers10[2 + 1]= { 1, 10, 100 }; int remove_leading_zeroes(int decimals, int var) { decimals%= 2; while (var < powers10[decimals--]) /* no-op */; return decimals; } The only allowed value for decimals is 1 and var must be > 0. Similar assertion is enforced by original function. So far I see nothing wrong with this code. I believe we should skip this patch, but still we should probably do something about these warning. What's your take? Regards, Sergey
Hi, Sergey! On May 28, Sergey Vojtovich wrote:
Hi Sergei and Alexey,
during our recent discussion re WebScaleSQL patches we agreed to review $subj: https://github.com/webscalesql/webscalesql-5.6/commit/094187c4a20e8d0c02f172...
All these warnings come from strings/decimal.c. I was able to reproduce them using gcc-4.8.1, cmake -DBUILD_CONFIG=mysql_release (basically needs -Wall -O3).
I simplified this function to the following: gcc -Wall -O3 -c dec.c -o dec.o
static const int powers10[2 + 1]= { 1, 10, 100 };
int remove_leading_zeroes(int decimals, int var) { decimals%= 2; while (var < powers10[decimals--]) /* no-op */; return decimals; }
The only allowed value for decimals is 1 and var must be > 0. Similar assertion is enforced by original function.
This is interesting. In fact, the compiler cannot know that var must be greater than zero, so I've made variables unsigned and added 0 to powers10 array: static const int powers10[]= { 0, 1, 10, 100 }; int remove_leading_zeroes(unsigned int decimals, unsigned int var) { decimals%= 2; while (var < powers10[decimals--]) ; return decimals; } This looks perfectly safe to me. But there is still a warning. Now, let's see the assembly: remove_leading_zeroes: andl $1, %edi movl %edi, %edx leal -1(%rdi), %eax cmpl powers10(,%rdx,4), %esi jae .L2 movl %eax, %ecx leal -2(%rdi), %edx cmpl powers10(,%rcx,4), %esi movl %edx, %eax jae .L2 leal -3(%rdi), %eax subl $4, %edi cmpl powers10(,%rdx,4), %esi cmovb %edi, %eax .L2: rep ret This roughly corresponds to decimals&= 1; if (var >= powers10[decimals ]) return decimals - 1; if (var >= powers10[decimals-1]) return decimals - 2; if (var >= powers10[decimals-2]) return decimals - 3; return decimals-4; The number of if's is equal to the number of elements in the array (adding more elements I've got more if's). Which, really, makes no sense, the size of the array is not used as a loop condition. I've no idea why gcc is doing that. A compiler bug? Regards, Sergei
Hi Sergei, I reported gcc bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61520 Regards, Sergey On Fri, Jun 13, 2014 at 05:55:43PM +0200, Sergei Golubchik wrote:
Hi, Sergey!
On May 28, Sergey Vojtovich wrote:
Hi Sergei and Alexey,
during our recent discussion re WebScaleSQL patches we agreed to review $subj: https://github.com/webscalesql/webscalesql-5.6/commit/094187c4a20e8d0c02f172...
All these warnings come from strings/decimal.c. I was able to reproduce them using gcc-4.8.1, cmake -DBUILD_CONFIG=mysql_release (basically needs -Wall -O3).
I simplified this function to the following: gcc -Wall -O3 -c dec.c -o dec.o
static const int powers10[2 + 1]= { 1, 10, 100 };
int remove_leading_zeroes(int decimals, int var) { decimals%= 2; while (var < powers10[decimals--]) /* no-op */; return decimals; }
The only allowed value for decimals is 1 and var must be > 0. Similar assertion is enforced by original function.
This is interesting. In fact, the compiler cannot know that var must be greater than zero, so I've made variables unsigned and added 0 to powers10 array:
static const int powers10[]= { 0, 1, 10, 100 }; int remove_leading_zeroes(unsigned int decimals, unsigned int var) { decimals%= 2; while (var < powers10[decimals--]) ; return decimals; }
This looks perfectly safe to me. But there is still a warning. Now, let's see the assembly:
remove_leading_zeroes: andl $1, %edi movl %edi, %edx leal -1(%rdi), %eax cmpl powers10(,%rdx,4), %esi jae .L2 movl %eax, %ecx leal -2(%rdi), %edx cmpl powers10(,%rcx,4), %esi movl %edx, %eax jae .L2 leal -3(%rdi), %eax subl $4, %edi cmpl powers10(,%rdx,4), %esi cmovb %edi, %eax .L2: rep ret
This roughly corresponds to
decimals&= 1; if (var >= powers10[decimals ]) return decimals - 1; if (var >= powers10[decimals-1]) return decimals - 2; if (var >= powers10[decimals-2]) return decimals - 3; return decimals-4;
The number of if's is equal to the number of elements in the array (adding more elements I've got more if's). Which, really, makes no sense, the size of the array is not used as a loop condition.
I've no idea why gcc is doing that. A compiler bug?
Regards, Sergei
participants (2)
-
Sergei Golubchik -
Sergey Vojtovich