Bug 79170 - [7 regression] memcmp builtin expansion sequence can overflow
Summary: [7 regression] memcmp builtin expansion sequence can overflow
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 7.0
: P3 normal
Target Milestone: 7.0
Assignee: acsawdey
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2017-01-20 21:00 UTC by acsawdey
Modified: 2017-03-27 15:40 UTC (History)
3 users (show)

See Also:
Host:
Target: powerpc64*-*-*
Build:
Known to work:
Known to fail:
Last reconfirmed: 2017-01-24 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description acsawdey 2017-01-20 21:00:50 UTC
The sequence generated for memcmp() builtin expansion can overflow in the subf and produce the wrong result.

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <limits.h>

#define SIZE 16
int main ()
{
  unsigned char buffer1[SIZE] = { 0,0,0,0,0,0,0,1 };
  unsigned char buffer2[SIZE] = { 0x80,0,0,0,0,0,0,3 };

  asm(" ");

  int n =  memcmp(buffer1,buffer2, SIZE);
  printf("%d\n", n);
}

produces 

        ldbrx 9,0,6
        ldbrx 4,0,8
        subf. 4,4,9
        bne 0,.L2
        addi 10,1,120
        ldbrx 9,0,10
        addi 10,1,104
        ldbrx 4,0,10
        subf 4,4,9
.L2:
        cntlzd 4,4
        addis 3,2,.LC0@toc@ha
        addi 3,3,.LC0@toc@l
        addi 4,4,-1
        xori 4,4,0x3f
        bl printf

If the subf result overflows such that the sign bit is not set in r4, then the wrong result is produced.
Comment 1 Martin Liška 2017-01-24 20:10:21 UTC
Probably same problem for:

#include <assert.h>

static unsigned char a[8] = {26, 54, 241, 144, 14, 86, 52, 58};
static unsigned char b[8] = {242, 38, 231, 126, 43, 254, 247, 41};

int main()
{
  if (__builtin_memcmp (a, b, 8) >= 0)
    __builtin_abort ();
}

Started with r240455.
Comment 2 acsawdey 2017-01-30 23:25:05 UTC
Author: acsawdey
Date: Mon Jan 30 23:24:24 2017
New Revision: 245041

URL: https://gcc.gnu.org/viewcvs?rev=245041&root=gcc&view=rev
Log:
2017-01-27  Aaron Sawdey  <acsawdey@linux.vnet.ibm.com>

	PR target/79170
	* gcc.dg/memcmp-1.c: Improved to catch failures seen in PR 79170.

2017-01-27  Aaron Sawdey  <acsawdey@linux.vnet.ibm.com>

	PR target/79170
	* config/rs6000/altivec.md (*setb_internal): Rename to setb_signed.
	(setb_unsigned) New pattern for setb with CCUNS.
	* config/rs6000/rs6000.c (expand_block_compare): Use a different
	subfc./subfe sequence to avoid overflow problems.  Generate a
	shorter sequence with cmpld/setb for power9.
	* config/rs6000/rs6000.md (subf<mode>3_carry_dot2): Add a new pattern
	for generating subfc. instruction.
	(cmpstrsi): Add TARGET_POPCNTD predicate as the generate sequence
	now uses this instruction.



Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/rs6000/altivec.md
    trunk/gcc/config/rs6000/rs6000.c
    trunk/gcc/config/rs6000/rs6000.md
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.dg/memcmp-1.c
Comment 3 Jakub Jelinek 2017-01-31 13:41:31 UTC
So fixed?
Comment 4 acsawdey 2017-01-31 16:34:51 UTC
Yes, should be fixed with 245041 -- different instruction sequence and a better memcmp/strncmp regtest to catch this and other things.
Comment 5 acsawdey 2017-01-31 16:36:18 UTC
Fixed in 245041.
Comment 6 acsawdey 2017-03-27 15:40:52 UTC
Author: acsawdey
Date: Mon Mar 27 15:40:20 2017
New Revision: 246504

URL: https://gcc.gnu.org/viewcvs?rev=246504&root=gcc&view=rev
Log:
2017-03-27  Aaron Sawdey  <acsawdey@linux.vnet.ibm.com>

	Backport from trunk
	PR target/79449
	PR target/79170

	* gcc.dg/strncmp-2.c: New.  Test strncmp and memcmp builtin expansion
	for reading beyond a 4k boundary.
	* config/rs6000/rs6000.c (expand_block_compare): Make sure runtime
	boundary crossing check and subsequent code generation agree.
	* gcc.dg/memcmp-1.c: Improved to catch failures seen in PR 79170.
	* config/rs6000/altivec.md (*setb_internal): Rename to setb_signed.
	(setb_unsigned) New pattern for setb with CCUNS.
	* config/rs6000/rs6000.c (expand_block_compare): Use a different
	subfc./subfe sequence to avoid overflow problems.  Generate a
	shorter sequence with cmpld/setb for power9.
	* config/rs6000/rs6000.md (subf<mode>3_carry_dot2): Add a new pattern
	for generating subfc. instruction.
	(cmpstrsi): Add TARGET_POPCNTD predicate as the generate sequence
	now uses this instruction.
	* config/rs6000/rs6000-protos.h (expand_strn_compare): Add arg.
	* config/rs6000/rs6000.c (expand_strn_compare): Add ability to expand
	strcmp. Fix bug where comparison didn't stop with zero byte. Fix
	case where N arg is SIZE_MAX.
	* config/rs6000/rs6000.md (cmpstrnsi): Args to expand_strn_compare.
	(cmpstrsi): Add pattern.
	* gcc.dg/memcmp-1.c: New.
	* gcc.dg/strncmp-1.c: New.
	* config/rs6000/rs6000-protos.h (expand_strn_compare): Declare.
	* config/rs6000/rs6000.md (UNSPEC_CMPB): New unspec.
	(cmpb<mode>3): pattern for generating cmpb.
	(cmpstrnsi): pattern to expand strncmp ().
	* config/rs6000/rs6000.opt (mstring-compare-inline-limit): Add a new
	target option for controlling how much code inline expansion of
	strncmp() will be allowed to generate.
	* config/rs6000/rs6000.c (expand_strncmp_align_check): generate code
	for runtime page crossing check of strncmp () args.
	(expand_strn_compare): Function to do builtin expansion of strncmp ().
	* config/i386/i386.md (cmpstrnsi): New test to bail out if neither
	string input is a string constant.
	* builtins.c (expand_builtin_strncmp): Attempt expansion of strncmp
	via cmpstrnsi even if neither string is constant.



Modified:
    branches/ibm/gcc-6-branch/gcc/ChangeLog.ibm
    branches/ibm/gcc-6-branch/gcc/builtins.c
    branches/ibm/gcc-6-branch/gcc/config/i386/i386.md
    branches/ibm/gcc-6-branch/gcc/config/rs6000/altivec.md
    branches/ibm/gcc-6-branch/gcc/config/rs6000/rs6000-protos.h
    branches/ibm/gcc-6-branch/gcc/config/rs6000/rs6000.c
    branches/ibm/gcc-6-branch/gcc/config/rs6000/rs6000.md
    branches/ibm/gcc-6-branch/gcc/config/rs6000/rs6000.opt