Fixing the test-suite broken after r246877

George Burgess IV via llvm-commits llvm-commits at lists.llvm.org
Mon Sep 7 19:29:39 PDT 2015 

> That doesn’t sound very safe :).

That's why I opted for noinline first. :)

> Let me run some benchmarks tomorrow and make sure there is no unexpected
performance impact. I am leaning towards turn off FORTIFY or using memcpy

SGTM.

On Mon, Sep 7, 2015 at 7:14 PM, Steven Wu <stevenwu at apple.com> wrote:

>
> On Sep 6, 2015, at 11:54 PM, George Burgess IV <
> george.burgess.iv at gmail.com> wrote:
>
> Hm. We can also make _strcpy_nochk always_inline. It doesn't *guarantee* a
> failure of __builtin_object_size, but it does require clang to lower to
> @llvm.objectsize, which is what was happening when nothing was failing.
> This way, we shouldn't end up dropping any perf on the floor, and we have a
> guaranteed solution for all platforms until someone makes @llvm.objectsize
> substantially smarter.
>
> That doesn’t sound very safe :). And you can play with the inline
> threshold in LLVM and break the benchmark.
>
>
> WRT Disabling FORTIFY: Ubuntu's default libc + Android's Bionic seem to
> both use -D_FORTIFY_SOURCE=0 to mean "turn off FORTIFY", so it's probably
> okay to assume that your proposed change will work well on most (if not
> all) platforms, as well.
>
> I'm happy with either solution. Please note that FORTIFY may slow things
> down a bit, so if we track things over time, then we might see a bit of a
> perf increase from the latter change.
>
> Let me run some benchmarks tomorrow and make sure there is no unexpected
> performance impact. I am leaning towards turn off FORTIFY or using memcpy.
>
>
>
> On Sun, Sep 6, 2015 at 11:18 PM, Steven Wu <stevenwu at apple.com> wrote:
>
>> Turning off inlining will hurt the performance for the cases when the
>> compiler can optimize strcpy functions and we do track performance for
>> test-suite.
>> On apple platform, the minimal change can just be turning off
>> FORTIFY_SOURCE for this benchmark. Do you know how your change will affect
>> other platforms?
>>
>> --- a/MultiSource/Benchmarks/MiBench/consumer-typeset/Makefile
>> +++ b/MultiSource/Benchmarks/MiBench/consumer-typeset/Makefile
>> @@ -1,7 +1,7 @@
>>  LEVEL = ../../../..
>>
>>  PROG     = consumer-typeset
>> -CPPFLAGS = -DOS_UNIX=1 -DOS_DOS=0 -DOS_MAC=0 -DDB_FIX=0 -DUSE_STAT=1
>> -DSAFE_DFT=0 -DCOLLATE=1 -DLIB_DIR=\"lout.lib\" -DFONT_DIR=\"font\"
>> -DMAPS_DIR=\"maps\" -DINCL_DIR=\"include\" -DDATA_DIR=\"data\"
>> -DHYPH_DIR=\"hyph\" -DLOCALE_DIR=\"locale\" -DCHAR_IN=1 -DCHAR_OUT=0
>> -DLOCALE_ON=1 -DASSERT_ON=1 -DDEBUG_ON=0  -DPDF_COMPRESSION=0
>> +CPPFLAGS = -DOS_UNIX=1 -DOS_DOS=0 -DOS_MAC=0 -DDB_FIX=0 -DUSE_STAT=1
>> -DSAFE_DFT=0 -DCOLLATE=1 -DLIB_DIR=\"lout.lib\" -DFONT_DIR=\"font\"
>> -DMAPS_DIR=\"maps\" -DINCL_DIR=\"include\" -DDATA_DIR=\"data\"
>> -DHYPH_DIR=\"hyph\" -DLOCALE_DIR=\"locale\" -DCHAR_IN=1 -DCHAR_OUT=0
>> -DLOCALE_ON=1 -DASSERT_ON=1 -DDEBUG_ON=0  -DPDF_COMPRESSION=0
>> -D_FORTIFY_SOURCE=0
>>  LDFLAGS  = -lm
>>  RUN_OPTIONS = -x -I $(PROJ_SRC_DIR)/data/include -D
>> $(PROJ_SRC_DIR)/data/data -F $(PROJ_SRC_DIR)/data/font -C
>> $(PROJ_SRC_DIR)/data/maps -H $(PROJ_OBJ_DIR)/data/hyph
>> $(PROJ_SRC_DIR)/large.lout
>>
>>
>> On Sep 6, 2015, at 10:40 PM, George Burgess IV <
>> george.burgess.iv at gmail.com> wrote:
>>
>> Well, we can lie by obfuscating things a bit. :)
>>
>> __builtin_object_size relies heavily on inlining to work well
>> across-functions. So, the following should make it always fail:
>>
>> static void __attribute__((noinline)) _strcpy_nochk(char *dst, char *src)
>> { strcpy(dst, src); }
>> #define StringCopy(a, b) _strcpy_nochk((char *)(a), (char *)(b))
>>
>>
>> On Sun, Sep 6, 2015 at 9:25 PM, Steven Wu <stevenwu at apple.com> wrote:
>>
>>>
>>> On Sep 6, 2015, at 6:38 PM, George Burgess IV <
>>> george.burgess.iv at gmail.com> wrote:
>>>
>>> Thanks for catching this!
>>>
>>> I'm not familiar with at all with test-suite, so someone else may be
>>> better suited to review your patch. That being said:
>>>
>>> id3tag.c -- LGTM
>>>
>>> externs.c -- Did some investigating. This code is rather different than
>>> what I'm used to. :)
>>>
>>>
>>> From the compiler's perspective: string(OBJECT) returns a pointer to a
>>> 4-char array inside of OBJECT. We try to copy a 119-byte string (file path)
>>> into this. memcpy() works here because Apple uses __builtin_object_size(p,
>>> 0) for memcpy, and __builtin_object_size(p, 1) (sometimes 0) for strcpy;
>>> the former has no clue what size OBJECT is, so it fails, while the latter
>>> says "hey, this field is clearly 4 bytes," causing FORTIFY to kill the
>>> program when we try to copy more than that. (relevant header:
>>> http://opensource.apple.com/source/Libc/Libc-1044.10.1/include/secure/_string.h
>>> )
>>>
>>> From the program's perspective at runtime, OBJECT points to a
>>> non-constant sized malloc. NewWord guarantees that *OBJECT has sufficient
>>> space to store the entire string. The types lie.
>>>
>>> In light of this, if we could add a comment to the new StringCopy
>>> definition that reads something like: "Types lie at some points in this
>>> program, and FORTIFY implementations rely on types. Using memcpy instead of
>>> strcpy here makes some FORTIFY impls permissive enough that they won't
>>> crash us when we're doing tricky (but still correct) things," that may be
>>> best. :)
>>>
>>>
>>> I guess there is no good way to lie to the type system that you can
>>> legally copy the string into 4 char array?
>>> How about we just #define StringCopy to __builtin___strcpy_chk with
>>>  __builtin_object_size(p, 0)? It might break -fno-builtin build though.
>>>
>>> Steven
>>>
>>>
>>>
>>> On Fri, Sep 4, 2015 at 11:25 PM, Steven Wu <stevenwu at apple.com> wrote:
>>>
>>>> r246877 exposes some undefined behavior in LNT that cause the test to
>>>> crash. Both consumer-lame and consumer-typeset has strcpy that overflow the
>>>> buffer, one intentionally one maybe by accident. Here is my proposed way to
>>>> fix the tests.
>>>>
>>>> Thanks
>>>>
>>>> Steven
>>>>
>>>>
>>>>
>>>>
>>>> From 7151bf31954c9d0680e4bb592dbc130d0234650b Mon Sep 17 00:00:00 2001
>>>> From: Steven Wu <stevenwu at apple.com>
>>>> Date: Fri, 4 Sep 2015 23:15:21 -0700
>>>> Subject: [PATCH] Fix the undefined behavior in MiBench
>>>>
>>>> The undefined behavior is causing runtime failure after r246877.
>>>> Fix consumer-lane by replacing strcpy with direct assignment to avoid
>>>> buffer overflow. consumer-typeset is intentionally overflowing buffer so
>>>> use memcpy instead of strcpy.
>>>> ---
>>>>  MultiSource/Benchmarks/MiBench/consumer-lame/id3tag.c     | 2 +-
>>>>  MultiSource/Benchmarks/MiBench/consumer-typeset/externs.h | 3 ++-
>>>>  2 files changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/MultiSource/Benchmarks/MiBench/consumer-lame/id3tag.c
>>>> b/MultiSource/Benchmarks/MiBench/consumer-lame/id3tag.c
>>>> index e24a966..23f2b86 100644
>>>> --- a/MultiSource/Benchmarks/MiBench/consumer-lame/id3tag.c
>>>> +++ b/MultiSource/Benchmarks/MiBench/consumer-lame/id3tag.c
>>>> @@ -34,7 +34,7 @@ void id3_inittag(ID3TAGDATA *tag) {
>>>>         strcpy( tag->album, "");
>>>>         strcpy( tag->year, "");
>>>>         strcpy( tag->comment, "");
>>>> -       strcpy( tag->genre, "ˇ");       /* unset genre */
>>>> +       tag->genre[0] = 'ˇ';    /* unset genre */
>>>>         tag->track = 0;
>>>>
>>>>         tag->valid = 0;         /* not ready for writing*/
>>>> diff --git a/MultiSource/Benchmarks/MiBench/consumer-typeset/externs.h
>>>> b/MultiSource/Benchmarks/MiBench/consumer-typeset/externs.h
>>>> index 7e050bd..a14e8ee 100644
>>>> --- a/MultiSource/Benchmarks/MiBench/consumer-typeset/externs.h
>>>> +++ b/MultiSource/Benchmarks/MiBench/consumer-typeset/externs.h
>>>> @@ -3266,7 +3266,8 @@ extern int          strcollcmp(char *a, char *b);
>>>>                     ( UseCollate ? strcollcmp((char *)(a),(char *)(b))
>>>> <= 0 \
>>>>                                  : strcmp((char *)(a),(char *)(b)) <= 0
>>>> )
>>>>  #define                  StringCat(a, b)       strcat((char
>>>> *)(a),(char *)(b))
>>>> -#define                  StringCopy(a, b)      strcpy((char
>>>> *)(a),(char *)(b))
>>>> +#define                  StringCopy(a, b)      (char*)memcpy((void
>>>> *)(a),(void *)(b), \
>>>> +                                             strlen((char*)(b)) + 1)
>>>>  #define                  StringLength(a)       strlen((char *)(a))
>>>>  #define                  StringFOpen(a, b)     fopen( (char *) (a),
>>>> (b) )
>>>>  #define                  StringFPuts(a, b)     fputs( (char *) (a),
>>>> (b) )
>>>> --
>>>> 2.3.8 (Apple Git-58)
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150907/90ee259e/attachment.html>

More information about the llvm-commits mailing list