Disabling ubsan's object size check at -O0

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Disabling ubsan's object size check at -O0

Xin Wang via cfe-dev
UBSan has an object size check (-fsanitize=object-size) which can determine when an object is not large enough to represent a value of its type. The check uses the @llvm.objectsize intrinsic to determine the size of objects.

AFAICT, and please let me know if I've missed something here, @llvm.objectsize always conservatively returns "I don't know" (i.e -1) at -O0, which means that it can't catch any issues at -O0. This is a problem because there is a substantial compile-time cost to enabling the object size check in debug builds. It seems unlikely that we can make @llvm.objectsize more precise at -O0 without regressing compile time and the debugging experience in other ways.

So, I'm proposing that we disable ubsan's object size check at -O0. This will speed up debug builds without compromising on diagnostic quality. E.g I measured a 26% decrease in the compile time for X86FastISel.cpp with this change, and a 32% decrease in the *.o size:

No ubsan [1]
-----------------
Average compile time: 5.27 s
X86FastISel.cpp.o size: 3.06 MB

Ubsan [2]
-------------
Average compile time: 9.49 s
X86FastISel.cpp.o size: 8.93 MB

Ubsan without the object size check [3]
----------------------------------------------------
Average compile time: 6.99 s
X86FastISel.cpp.o size: 6.06 MB

There's reason to expect similar compile-time / binary size savings with other *.cpp files. The object size check is in the same category of checks as the null check and the alignment check. This group of checks accounts for the vast majority of checks inserted by ubsan (over 90% in some macOS apps), so any savings here would be helpful.

Any objections? Anyone else in favor?

thanks,
vedant

[1] -O0 -g
[2] -O0 -g -fsanitize=undefined -fno-sanitize-recover=all -fno-sanitize=vptr,function
[3] -O0 -g -fsanitize=undefined -fno-sanitize-recover=all -fno-sanitize=vptr,function,object-size

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Disabling ubsan's object size check at -O0

Xin Wang via cfe-dev
Hi,

> AFAICT, and please let me know if I've missed something here, @llvm.objectsize always conservatively returns "I don't know" (i.e -1) at -O0

Yup. The object-size sanitizer lowers directly to @llvm.objectsize
(bypassing clang's __builtin_object_size evaluator, which is the
right thing to do here), and the only places we try to lower
@llvm.objectsize to a non-default value, to my knowledge, are
in InstCombine and CodeGenPrepare. Looks like both of those
are disabled on -O0.

As you mentioned, if we did decide that we should try to
lower @llvm.objectsize calls more aggressively in -O0, I'd
imagine we'd still end up handing back -1 in ~99% of cases.
(With the remaining 1% looking something like:

struct Foo {int i;};
char c;
int bar() { return reinterpret_cast<Foo *>(&c)->i; } // error: c is
too small, ...
)

So, unless I'm missing something, I think this is a pure win for
ubsan in -O0. I'm in favor of it.

Thank you!
George

On Tue, Jun 20, 2017 at 4:57 PM, Vedant Kumar via cfe-dev
<[hidden email]> wrote:

> UBSan has an object size check (-fsanitize=object-size) which can determine
> when an object is not large enough to represent a value of its type. The
> check uses the @llvm.objectsize intrinsic to determine the size of objects.
>
> AFAICT, and please let me know if I've missed something here,
> @llvm.objectsize always conservatively returns "I don't know" (i.e -1) at
> -O0, which means that it can't catch any issues at -O0. This is a problem
> because there is a substantial compile-time cost to enabling the object size
> check in debug builds. It seems unlikely that we can make @llvm.objectsize
> more precise at -O0 without regressing compile time and the debugging
> experience in other ways.
>
> So, I'm proposing that we disable ubsan's object size check at -O0. This
> will speed up debug builds without compromising on diagnostic quality. E.g I
> measured a 26% decrease in the compile time for X86FastISel.cpp with this
> change, and a 32% decrease in the *.o size:
>
> No ubsan [1]
> -----------------
> Average compile time: 5.27 s
> X86FastISel.cpp.o size: 3.06 MB
>
> Ubsan [2]
> -------------
> Average compile time: 9.49 s
> X86FastISel.cpp.o size: 8.93 MB
>
> Ubsan without the object size check [3]
> ----------------------------------------------------
> Average compile time: 6.99 s
> X86FastISel.cpp.o size: 6.06 MB
>
>
> There's reason to expect similar compile-time / binary size savings with
> other *.cpp files. The object size check is in the same category of checks
> as the null check and the alignment check. This group of checks accounts for
> the vast majority of checks inserted by ubsan (over 90% in some macOS apps),
> so any savings here would be helpful.
>
> Any objections? Anyone else in favor?
>
> thanks,
> vedant
>
> [1] -O0 -g
> [2] -O0 -g -fsanitize=undefined -fno-sanitize-recover=all
> -fno-sanitize=vptr,function
> [3] -O0 -g -fsanitize=undefined -fno-sanitize-recover=all
> -fno-sanitize=vptr,function,object-size
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Disabling ubsan's object size check at -O0

Xin Wang via cfe-dev
This makes sense to me.

On 20 June 2017 at 17:58, George Burgess IV via cfe-dev <[hidden email]> wrote:
Hi,

> AFAICT, and please let me know if I've missed something here, @llvm.objectsize always conservatively returns "I don't know" (i.e -1) at -O0

Yup. The object-size sanitizer lowers directly to @llvm.objectsize
(bypassing clang's __builtin_object_size evaluator, which is the
right thing to do here), and the only places we try to lower
@llvm.objectsize to a non-default value, to my knowledge, are
in InstCombine and CodeGenPrepare. Looks like both of those
are disabled on -O0.

As you mentioned, if we did decide that we should try to
lower @llvm.objectsize calls more aggressively in -O0, I'd
imagine we'd still end up handing back -1 in ~99% of cases.
(With the remaining 1% looking something like:

struct Foo {int i;};
char c;
int bar() { return reinterpret_cast<Foo *>(&c)->i; } // error: c is
too small, ...
)

So, unless I'm missing something, I think this is a pure win for
ubsan in -O0. I'm in favor of it.

Thank you!
George

On Tue, Jun 20, 2017 at 4:57 PM, Vedant Kumar via cfe-dev
<[hidden email]> wrote:
> UBSan has an object size check (-fsanitize=object-size) which can determine
> when an object is not large enough to represent a value of its type. The
> check uses the @llvm.objectsize intrinsic to determine the size of objects.
>
> AFAICT, and please let me know if I've missed something here,
> @llvm.objectsize always conservatively returns "I don't know" (i.e -1) at
> -O0, which means that it can't catch any issues at -O0. This is a problem
> because there is a substantial compile-time cost to enabling the object size
> check in debug builds. It seems unlikely that we can make @llvm.objectsize
> more precise at -O0 without regressing compile time and the debugging
> experience in other ways.
>
> So, I'm proposing that we disable ubsan's object size check at -O0. This
> will speed up debug builds without compromising on diagnostic quality. E.g I
> measured a 26% decrease in the compile time for X86FastISel.cpp with this
> change, and a 32% decrease in the *.o size:
>
> No ubsan [1]
> -----------------
> Average compile time: 5.27 s
> X86FastISel.cpp.o size: 3.06 MB
>
> Ubsan [2]
> -------------
> Average compile time: 9.49 s
> X86FastISel.cpp.o size: 8.93 MB
>
> Ubsan without the object size check [3]
> ----------------------------------------------------
> Average compile time: 6.99 s
> X86FastISel.cpp.o size: 6.06 MB
>
>
> There's reason to expect similar compile-time / binary size savings with
> other *.cpp files. The object size check is in the same category of checks
> as the null check and the alignment check. This group of checks accounts for
> the vast majority of checks inserted by ubsan (over 90% in some macOS apps),
> so any savings here would be helpful.
>
> Any objections? Anyone else in favor?
>
> thanks,
> vedant
>
> [1] -O0 -g
> [2] -O0 -g -fsanitize=undefined -fno-sanitize-recover=all
> -fno-sanitize=vptr,function
> [3] -O0 -g -fsanitize=undefined -fno-sanitize-recover=all
> -fno-sanitize=vptr,function,object-size
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Disabling ubsan's object size check at -O0

Xin Wang via cfe-dev
In reply to this post by Xin Wang via cfe-dev
George Burgess IV via cfe-dev <[hidden email]> writes:

> Hi,
>
>> AFAICT, and please let me know if I've missed something here,
>> @llvm.objectsize always conservatively returns "I don't know" (i.e
>> -1) at -O0
>
> Yup. The object-size sanitizer lowers directly to @llvm.objectsize
> (bypassing clang's __builtin_object_size evaluator, which is the
> right thing to do here), and the only places we try to lower
> @llvm.objectsize to a non-default value, to my knowledge, are
> in InstCombine and CodeGenPrepare. Looks like both of those
> are disabled on -O0.
>
> As you mentioned, if we did decide that we should try to
> lower @llvm.objectsize calls more aggressively in -O0, I'd
> imagine we'd still end up handing back -1 in ~99% of cases.
> (With the remaining 1% looking something like:
>
> struct Foo {int i;};
> char c;
> int bar() { return reinterpret_cast<Foo *>(&c)->i; } // error: c is
> too small, ...
> )

How reasonable would it be to do something simpler that catches this in
particular? Though, ISTM this is probably obvious enough that we could
catch it at compile time.

> So, unless I'm missing something, I think this is a pure win for
> ubsan in -O0. I'm in favor of it.
>
> Thank you!
> George
>
> On Tue, Jun 20, 2017 at 4:57 PM, Vedant Kumar via cfe-dev
> <[hidden email]> wrote:
>> UBSan has an object size check (-fsanitize=object-size) which can determine
>> when an object is not large enough to represent a value of its type. The
>> check uses the @llvm.objectsize intrinsic to determine the size of objects.
>>
>> AFAICT, and please let me know if I've missed something here,
>> @llvm.objectsize always conservatively returns "I don't know" (i.e -1) at
>> -O0, which means that it can't catch any issues at -O0. This is a problem
>> because there is a substantial compile-time cost to enabling the object size
>> check in debug builds. It seems unlikely that we can make @llvm.objectsize
>> more precise at -O0 without regressing compile time and the debugging
>> experience in other ways.
>>
>> So, I'm proposing that we disable ubsan's object size check at -O0. This
>> will speed up debug builds without compromising on diagnostic quality. E.g I
>> measured a 26% decrease in the compile time for X86FastISel.cpp with this
>> change, and a 32% decrease in the *.o size:
>>
>> No ubsan [1]
>> -----------------
>> Average compile time: 5.27 s
>> X86FastISel.cpp.o size: 3.06 MB
>>
>> Ubsan [2]
>> -------------
>> Average compile time: 9.49 s
>> X86FastISel.cpp.o size: 8.93 MB
>>
>> Ubsan without the object size check [3]
>> ----------------------------------------------------
>> Average compile time: 6.99 s
>> X86FastISel.cpp.o size: 6.06 MB
>>
>>
>> There's reason to expect similar compile-time / binary size savings with
>> other *.cpp files. The object size check is in the same category of checks
>> as the null check and the alignment check. This group of checks accounts for
>> the vast majority of checks inserted by ubsan (over 90% in some macOS apps),
>> so any savings here would be helpful.
>>
>> Any objections? Anyone else in favor?
>>
>> thanks,
>> vedant
>>
>> [1] -O0 -g
>> [2] -O0 -g -fsanitize=undefined -fno-sanitize-recover=all
>> -fno-sanitize=vptr,function
>> [3] -O0 -g -fsanitize=undefined -fno-sanitize-recover=all
>> -fno-sanitize=vptr,function,object-size
>>
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Disabling ubsan's object size check at -O0

Xin Wang via cfe-dev

On Jun 20, 2017, at 11:33 PM, Justin Bogner <[hidden email]> wrote:

George Burgess IV via cfe-dev <[hidden email]> writes:
Hi,

AFAICT, and please let me know if I've missed something here,
@llvm.objectsize always conservatively returns "I don't know" (i.e
-1) at -O0

Yup. The object-size sanitizer lowers directly to @llvm.objectsize
(bypassing clang's __builtin_object_size evaluator, which is the
right thing to do here), and the only places we try to lower
@llvm.objectsize to a non-default value, to my knowledge, are
in InstCombine and CodeGenPrepare. Looks like both of those
are disabled on -O0.

As you mentioned, if we did decide that we should try to
lower @llvm.objectsize calls more aggressively in -O0, I'd
imagine we'd still end up handing back -1 in ~99% of cases.
(With the remaining 1% looking something like:

struct Foo {int i;};
char c;
int bar() { return reinterpret_cast<Foo *>(&c)->i; } // error: c is
too small, ...
)

How reasonable would it be to do something simpler that catches this in
particular? Though, ISTM this is probably obvious enough that we could
catch it at compile time.

It would be feasible to warn at compile-time for simple cases like this one, or the one in [1]. We currently don't: at least, not in the static analyzer or with -Wall -Wextra.

That said, I'm not sure that this "simple" type of violation is commonplace. All of the -fsanitize=object-size violations I've seen involve either heap allocation or cross-TU calls. I imagine that these bugs would be hard to diagnose at compile-time.


vedant


So, unless I'm missing something, I think this is a pure win for
ubsan in -O0. I'm in favor of it.

Thank you!
George

On Tue, Jun 20, 2017 at 4:57 PM, Vedant Kumar via cfe-dev
<[hidden email]> wrote:
UBSan has an object size check (-fsanitize=object-size) which can determine
when an object is not large enough to represent a value of its type. The
check uses the @llvm.objectsize intrinsic to determine the size of objects.

AFAICT, and please let me know if I've missed something here,
@llvm.objectsize always conservatively returns "I don't know" (i.e -1) at
-O0, which means that it can't catch any issues at -O0. This is a problem
because there is a substantial compile-time cost to enabling the object size
check in debug builds. It seems unlikely that we can make @llvm.objectsize
more precise at -O0 without regressing compile time and the debugging
experience in other ways.

So, I'm proposing that we disable ubsan's object size check at -O0. This
will speed up debug builds without compromising on diagnostic quality. E.g I
measured a 26% decrease in the compile time for X86FastISel.cpp with this
change, and a 32% decrease in the *.o size:

No ubsan [1]
-----------------
Average compile time: 5.27 s
X86FastISel.cpp.o size: 3.06 MB

Ubsan [2]
-------------
Average compile time: 9.49 s
X86FastISel.cpp.o size: 8.93 MB

Ubsan without the object size check [3]
----------------------------------------------------
Average compile time: 6.99 s
X86FastISel.cpp.o size: 6.06 MB


There's reason to expect similar compile-time / binary size savings with
other *.cpp files. The object size check is in the same category of checks
as the null check and the alignment check. This group of checks accounts for
the vast majority of checks inserted by ubsan (over 90% in some macOS apps),
so any savings here would be helpful.

Any objections? Anyone else in favor?

thanks,
vedant

[1] -O0 -g
[2] -O0 -g -fsanitize=undefined -fno-sanitize-recover=all
-fno-sanitize=vptr,function
[3] -O0 -g -fsanitize=undefined -fno-sanitize-recover=all
-fno-sanitize=vptr,function,object-size

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Loading...