Re: r281277 - [Sema] Fix PR30346: relax __builtin_object_size checks.
- Original Message - > From: "George Burgess IV" <george.burgess...@gmail.com> > To: "Hal Finkel" <hfin...@anl.gov> > Cc: "Richard Smith" <rich...@metafoo.co.uk>, "Joerg Sonnenberger" > <jo...@bec.de>, "cfe-commits" > <cfe-commits@lists.llvm.org> > Sent: Monday, September 19, 2016 11:21:33 PM > Subject: Re: r281277 - [Sema] Fix PR30346: relax __builtin_object_size checks. > > > WFM; I'll put together a patch that only allows this under > -fno-strict-aliasing. > > > I'm entirely unfamiliar with struct-path-tbaa, so Hal, do you see a > reason why struct-path-tbaa wouldn't play nicely with flexible > arrays at the end of types? Glancing at it, I don't think it should > cause problems, but a more authoritative answer would really be > appreciated. :) If it might cause issues now or in the future, I'm > happy to be conservative here if -fno-strict-path-tbaa is given, > too. We currently don't emit struct-path-tbaa for array members. We likely should, and we'll need to keep flexible array members in mind when we implement that extension. I don't think that the current representation has a way to represent an unbounded size (except for using ((size_t) -1), which might be as good as anything else). -Hal > > On Tue, Sep 13, 2016 at 2:00 PM, Joerg Sonnenberger via cfe-commits < > cfe-commits@lists.llvm.org > wrote: > > > On Tue, Sep 13, 2016 at 12:51:52PM -0700, Richard Smith wrote: > > On Tue, Sep 13, 2016 at 10:44 AM, Joerg Sonnenberger via > > cfe-commits < > > cfe-commits@lists.llvm.org > wrote: > > > > > IMO this should be restricted to code that explicitly disables > > > C/C++ > > > aliasing rules. > > > > > > Do you mean -fno-strict-aliasing or -fno-struct-path-tbaa or > > something else > > here? (I think we're not doing anyone any favours by making > > _FORTIFY_SOURCE > > say that a pattern is OK in cases when LLVM will in fact optimize > > on the > > assumption that it's UB, but I don't recall how aggressive > > -fstruct-path-tbaa is for trailing array members.) > > The former immediately, the latter potentially as well. I can't think > of > many use cases for this kind of idiom that don't involve type > prunning > and socket code is notoriously bad in that regard by necessity. > > > > Joerg > ___ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > > -- Hal Finkel Lead, Compiler Technology and Programming Languages Leadership Computing Facility Argonne National Laboratory ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r281277 - [Sema] Fix PR30346: relax __builtin_object_size checks.
Noted; thanks for the correction. :) On Tue, Sep 20, 2016 at 3:04 AM, Joerg Sonnenberger via cfe-commits < cfe-commits@lists.llvm.org> wrote: > On Mon, Sep 19, 2016 at 09:21:33PM -0700, George Burgess IV wrote: > > I'm entirely unfamiliar with struct-path-tbaa, so Hal, do you see a > reason > > why struct-path-tbaa wouldn't play nicely with flexible arrays at the end > > of types? Glancing at it, I don't think it should cause problems, but a > > more authoritative answer would really be appreciated. :) If it might > cause > > issues now or in the future, I'm happy to be conservative here if > > -fno-strict-path-tbaa is given, too. > > Please don't call them flexible types. That's a misname. The standard > provides a clear mechanism for arrays with statically undefined size -- > which is providing no size at all. We do provide the same support for > array size of 1 for legacy compat. Any other size is basically abuse. > > Joerg > ___ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r281277 - [Sema] Fix PR30346: relax __builtin_object_size checks.
On Mon, Sep 19, 2016 at 09:21:33PM -0700, George Burgess IV wrote: > I'm entirely unfamiliar with struct-path-tbaa, so Hal, do you see a reason > why struct-path-tbaa wouldn't play nicely with flexible arrays at the end > of types? Glancing at it, I don't think it should cause problems, but a > more authoritative answer would really be appreciated. :) If it might cause > issues now or in the future, I'm happy to be conservative here if > -fno-strict-path-tbaa is given, too. Please don't call them flexible types. That's a misname. The standard provides a clear mechanism for arrays with statically undefined size -- which is providing no size at all. We do provide the same support for array size of 1 for legacy compat. Any other size is basically abuse. Joerg ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r281277 - [Sema] Fix PR30346: relax __builtin_object_size checks.
WFM; I'll put together a patch that only allows this under -fno-strict-aliasing. I'm entirely unfamiliar with struct-path-tbaa, so Hal, do you see a reason why struct-path-tbaa wouldn't play nicely with flexible arrays at the end of types? Glancing at it, I don't think it should cause problems, but a more authoritative answer would really be appreciated. :) If it might cause issues now or in the future, I'm happy to be conservative here if -fno-strict-path-tbaa is given, too. On Tue, Sep 13, 2016 at 2:00 PM, Joerg Sonnenberger via cfe-commits < cfe-commits@lists.llvm.org> wrote: > On Tue, Sep 13, 2016 at 12:51:52PM -0700, Richard Smith wrote: > > On Tue, Sep 13, 2016 at 10:44 AM, Joerg Sonnenberger via cfe-commits < > > cfe-commits@lists.llvm.org> wrote: > > > > > IMO this should be restricted to code that explicitly disables C/C++ > > > aliasing rules. > > > > > > Do you mean -fno-strict-aliasing or -fno-struct-path-tbaa or something > else > > here? (I think we're not doing anyone any favours by making > _FORTIFY_SOURCE > > say that a pattern is OK in cases when LLVM will in fact optimize on the > > assumption that it's UB, but I don't recall how aggressive > > -fstruct-path-tbaa is for trailing array members.) > > The former immediately, the latter potentially as well. I can't think of > many use cases for this kind of idiom that don't involve type prunning > and socket code is notoriously bad in that regard by necessity. > > Joerg > ___ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r281277 - [Sema] Fix PR30346: relax __builtin_object_size checks.
On Tue, Sep 13, 2016 at 12:51:52PM -0700, Richard Smith wrote: > On Tue, Sep 13, 2016 at 10:44 AM, Joerg Sonnenberger via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > > > IMO this should be restricted to code that explicitly disables C/C++ > > aliasing rules. > > > Do you mean -fno-strict-aliasing or -fno-struct-path-tbaa or something else > here? (I think we're not doing anyone any favours by making _FORTIFY_SOURCE > say that a pattern is OK in cases when LLVM will in fact optimize on the > assumption that it's UB, but I don't recall how aggressive > -fstruct-path-tbaa is for trailing array members.) The former immediately, the latter potentially as well. I can't think of many use cases for this kind of idiom that don't involve type prunning and socket code is notoriously bad in that regard by necessity. Joerg ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r281277 - [Sema] Fix PR30346: relax __builtin_object_size checks.
Yeah, this patch didn't give me the warm fuzzies, either.
AFAICT, our only other options are having some sort of struct whitelist
(either hard-coded, or given as a flag), or telling people to turn
_FORTIFY_SOURCE down if they have code that looks like this. Given that
this is apparently common in the world of BSD, I'm unsure how many projects
this would impact if we chose "turn _FORTIFY_SOURCE down." So, this
solution seemed like the least bad of the above.
If people feel differently or have other ideas, I'm happy to add
restrictions/try a different approach. :)
On Tue, Sep 13, 2016 at 12:51 PM, Richard Smith via cfe-commits <
cfe-commits@lists.llvm.org> wrote:
> On Tue, Sep 13, 2016 at 10:44 AM, Joerg Sonnenberger via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> On Mon, Sep 12, 2016 at 11:50:36PM -, George Burgess IV via
>> cfe-commits wrote:
>> > Author: gbiv
>> > Date: Mon Sep 12 18:50:35 2016
>> > New Revision: 281277
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=281277=rev
>> > Log:
>> > [Sema] Fix PR30346: relax __builtin_object_size checks.
>> >
>> > This patch makes us act more conservatively when trying to determine
>> > the objectsize for an array at the end of an object. This is in
>> > response to code like the following:
>> >
>> > ```
>> > struct sockaddr {
>> > /* snip */
>> > char sa_data[14];
>> > };
>> >
>> > void foo(const char *s) {
>> > size_t slen = strlen(s) + 1;
>> > size_t added_len = slen <= 14 ? 0 : slen - 14;
>> > struct sockaddr *sa = malloc(sizeof(struct sockaddr) + added_len);
>> > strcpy(sa->sa_data, s);
>> > // ...
>> > }
>> > ```
>> >
>> > `__builtin_object_size(sa->sa_data, 1)` would return 14, when there
>> > could be more than 14 bytes at `sa->sa_data`.
>> >
>> > Code like this is apparently not uncommon. FreeBSD's manual even
>> > explicitly mentions this pattern:
>> > https://www.freebsd.org/doc/en/books/developers-handbook/soc
>> kets-essential-functions.html
>> > (section 7.5.1.1.2).
>> >
>> > In light of this, we now just give up on any array at the end of an
>> > object if we can't find the object's initial allocation.
>> >
>> > I lack numbers for how much more conservative we actually become as a
>> > result of this change, so I chose the fix that would make us as
>> > compatible with GCC as possible. If we want to be more aggressive, I'm
>> > happy to consider some kind of whitelist or something instead.
>>
>> IMO this should be restricted to code that explicitly disables C/C++
>> aliasing rules.
>
>
> Do you mean -fno-strict-aliasing or -fno-struct-path-tbaa or something
> else here? (I think we're not doing anyone any favours by making
> _FORTIFY_SOURCE say that a pattern is OK in cases when LLVM will in fact
> optimize on the assumption that it's UB, but I don't recall how aggressive
> -fstruct-path-tbaa is for trailing array members.)
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r281277 - [Sema] Fix PR30346: relax __builtin_object_size checks.
On Tue, Sep 13, 2016 at 10:44 AM, Joerg Sonnenberger via cfe-commits <
cfe-commits@lists.llvm.org> wrote:
> On Mon, Sep 12, 2016 at 11:50:36PM -, George Burgess IV via
> cfe-commits wrote:
> > Author: gbiv
> > Date: Mon Sep 12 18:50:35 2016
> > New Revision: 281277
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=281277=rev
> > Log:
> > [Sema] Fix PR30346: relax __builtin_object_size checks.
> >
> > This patch makes us act more conservatively when trying to determine
> > the objectsize for an array at the end of an object. This is in
> > response to code like the following:
> >
> > ```
> > struct sockaddr {
> > /* snip */
> > char sa_data[14];
> > };
> >
> > void foo(const char *s) {
> > size_t slen = strlen(s) + 1;
> > size_t added_len = slen <= 14 ? 0 : slen - 14;
> > struct sockaddr *sa = malloc(sizeof(struct sockaddr) + added_len);
> > strcpy(sa->sa_data, s);
> > // ...
> > }
> > ```
> >
> > `__builtin_object_size(sa->sa_data, 1)` would return 14, when there
> > could be more than 14 bytes at `sa->sa_data`.
> >
> > Code like this is apparently not uncommon. FreeBSD's manual even
> > explicitly mentions this pattern:
> > https://www.freebsd.org/doc/en/books/developers-handbook/
> sockets-essential-functions.html
> > (section 7.5.1.1.2).
> >
> > In light of this, we now just give up on any array at the end of an
> > object if we can't find the object's initial allocation.
> >
> > I lack numbers for how much more conservative we actually become as a
> > result of this change, so I chose the fix that would make us as
> > compatible with GCC as possible. If we want to be more aggressive, I'm
> > happy to consider some kind of whitelist or something instead.
>
> IMO this should be restricted to code that explicitly disables C/C++
> aliasing rules.
Do you mean -fno-strict-aliasing or -fno-struct-path-tbaa or something else
here? (I think we're not doing anyone any favours by making _FORTIFY_SOURCE
say that a pattern is OK in cases when LLVM will in fact optimize on the
assumption that it's UB, but I don't recall how aggressive
-fstruct-path-tbaa is for trailing array members.)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r281277 - [Sema] Fix PR30346: relax __builtin_object_size checks.
On Mon, Sep 12, 2016 at 11:50:36PM -, George Burgess IV via cfe-commits
wrote:
> Author: gbiv
> Date: Mon Sep 12 18:50:35 2016
> New Revision: 281277
>
> URL: http://llvm.org/viewvc/llvm-project?rev=281277=rev
> Log:
> [Sema] Fix PR30346: relax __builtin_object_size checks.
>
> This patch makes us act more conservatively when trying to determine
> the objectsize for an array at the end of an object. This is in
> response to code like the following:
>
> ```
> struct sockaddr {
> /* snip */
> char sa_data[14];
> };
>
> void foo(const char *s) {
> size_t slen = strlen(s) + 1;
> size_t added_len = slen <= 14 ? 0 : slen - 14;
> struct sockaddr *sa = malloc(sizeof(struct sockaddr) + added_len);
> strcpy(sa->sa_data, s);
> // ...
> }
> ```
>
> `__builtin_object_size(sa->sa_data, 1)` would return 14, when there
> could be more than 14 bytes at `sa->sa_data`.
>
> Code like this is apparently not uncommon. FreeBSD's manual even
> explicitly mentions this pattern:
> https://www.freebsd.org/doc/en/books/developers-handbook/sockets-essential-functions.html
> (section 7.5.1.1.2).
>
> In light of this, we now just give up on any array at the end of an
> object if we can't find the object's initial allocation.
>
> I lack numbers for how much more conservative we actually become as a
> result of this change, so I chose the fix that would make us as
> compatible with GCC as possible. If we want to be more aggressive, I'm
> happy to consider some kind of whitelist or something instead.
IMO this should be restricted to code that explicitly disables C/C++
aliasing rules.
Joerg
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r281277 - [Sema] Fix PR30346: relax __builtin_object_size checks.
Author: gbiv
Date: Mon Sep 12 18:50:35 2016
New Revision: 281277
URL: http://llvm.org/viewvc/llvm-project?rev=281277=rev
Log:
[Sema] Fix PR30346: relax __builtin_object_size checks.
This patch makes us act more conservatively when trying to determine
the objectsize for an array at the end of an object. This is in
response to code like the following:
```
struct sockaddr {
/* snip */
char sa_data[14];
};
void foo(const char *s) {
size_t slen = strlen(s) + 1;
size_t added_len = slen <= 14 ? 0 : slen - 14;
struct sockaddr *sa = malloc(sizeof(struct sockaddr) + added_len);
strcpy(sa->sa_data, s);
// ...
}
```
`__builtin_object_size(sa->sa_data, 1)` would return 14, when there
could be more than 14 bytes at `sa->sa_data`.
Code like this is apparently not uncommon. FreeBSD's manual even
explicitly mentions this pattern:
https://www.freebsd.org/doc/en/books/developers-handbook/sockets-essential-functions.html
(section 7.5.1.1.2).
In light of this, we now just give up on any array at the end of an
object if we can't find the object's initial allocation.
I lack numbers for how much more conservative we actually become as a
result of this change, so I chose the fix that would make us as
compatible with GCC as possible. If we want to be more aggressive, I'm
happy to consider some kind of whitelist or something instead.
Modified:
cfe/trunk/lib/AST/ExprConstant.cpp
cfe/trunk/test/CodeGen/object-size.c
cfe/trunk/test/CodeGen/pass-object-size.c
Modified: cfe/trunk/lib/AST/ExprConstant.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ExprConstant.cpp?rev=281277=281276=281277=diff
==
--- cfe/trunk/lib/AST/ExprConstant.cpp (original)
+++ cfe/trunk/lib/AST/ExprConstant.cpp Mon Sep 12 18:50:35 2016
@@ -6826,14 +6826,21 @@ static bool tryEvaluateBuiltinObjectSize
// struct Foo *F = (struct Foo *)malloc(sizeof(struct Foo) + strlen(Bar));
// strcpy(>c[0], Bar);
//
- // So, if we see that we're examining a 1-length (or 0-length) array at the
- // end of a struct with an unknown base, we give up instead of breaking code
- // that behaves this way. Note that we only do this when Type=1, because
- // Type=3 is a lower bound, so answering conservatively is fine.
+ // So, if we see that we're examining an array at the end of a struct with an
+ // unknown base, we give up instead of breaking code that behaves this way.
+ // Note that we only do this when Type=1, because Type=3 is a lower bound, so
+ // answering conservatively is fine.
+ //
+ // We used to be a bit more aggressive here; we'd only be conservative if the
+ // array at the end was flexible, or if it had 0 or 1 elements. This broke
+ // some common standard library extensions (PR30346), but was otherwise
+ // seemingly fine. It may be useful to reintroduce this behavior with some
+ // sort of whitelist. OTOH, it seems that GCC is always conservative with the
+ // last element in structs (if it's an array), so our current behavior is
more
+ // compatible than a whitelisting approach would be.
if (End.InvalidBase && SubobjectOnly && Type == 1 &&
End.Designator.Entries.size() == End.Designator.MostDerivedPathLength &&
End.Designator.MostDerivedIsArrayElement &&
- End.Designator.MostDerivedArraySize < 2 &&
isDesignatorAtObjectEnd(Info.Ctx, End))
return false;
Modified: cfe/trunk/test/CodeGen/object-size.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/object-size.c?rev=281277=281276=281277=diff
==
--- cfe/trunk/test/CodeGen/object-size.c (original)
+++ cfe/trunk/test/CodeGen/object-size.c Mon Sep 12 18:50:35 2016
@@ -276,7 +276,7 @@ void test23(struct Test23Ty *p) {
// CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false)
gi = __builtin_object_size(>t[5], 0);
- // CHECK: store i32 20
+ // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false)
gi = __builtin_object_size(>t[5], 1);
// CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 true)
gi = __builtin_object_size(>t[5], 2);
@@ -444,7 +444,7 @@ void test29(struct DynStructVar *dv, str
// CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false)
gi = __builtin_object_size(ss->snd, 0);
- // CHECK: store i32 2
+ // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false)
gi = __builtin_object_size(ss->snd, 1);
// CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 true)
gi = __builtin_object_size(ss->snd, 2);
@@ -505,7 +505,7 @@ void test31() {
// CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false)
gi = __builtin_object_size(ds1[9].snd, 1);
- // CHECK: store i32 2
+ // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false)
gi = __builtin_object_size([9].snd[0], 1);
// CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1
