[llvm] [clang] [Clang] Correct __builtin_dynamic_object_size for subobject types (PR #78526)
https://github.com/bwendling closed https://github.com/llvm/llvm-project/pull/78526 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] [Clang] Correct __builtin_dynamic_object_size for subobject types (PR #78526)
zygoloid wrote: It might be academic at this point, but for what it's worth, `__builtin_dynamic_object_size` is not a GCC builtin that clang copied, it's [a clang builtin](https://reviews.llvm.org/D56760) that GCC copied. https://github.com/llvm/llvm-project/pull/78526 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] [Clang] Correct __builtin_dynamic_object_size for subobject types (PR #78526)
efriedma-quic wrote:
> ```c
> struct x {
> int a;
> char foo[2][40];
> int b;
> int c;
> };
>
> size_t f(struct x *p, int idx) {
> return __builtin_dynamic_object_size(>foo[idx], 1);
> }
> ```
If I'm following correctly, the return here is 0, 40, or 80, depending on the
value of idx? That's not a constant, but the computation is entirely
syntactic; it doesn't matter what "p" actually points to. So clang can lower
the builtin itself. Currently it doesn't, I think, because all the relevant
code is in ExprConstant, but the code could be adapted.
The problem, really, is that we can't easily extend that approach to stuff like
the following:
```c
size_t f(struct x *p, int idx) {
char *c = >foo[idx];
return __builtin_dynamic_object_size(c, 1);
}
```
https://github.com/llvm/llvm-project/pull/78526
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] [Clang] Correct __builtin_dynamic_object_size for subobject types (PR #78526)
efriedma-quic wrote: > maybe we could add the subtype as part of the llvm.objectsize intrinsic and > use that instead of grappling with the whole object's type I'm not sure I follow; if you know the object's type, doesn't that mean you also know its size? >(I don't readily know of any transformation that changes a structure's layout. >Does it exist?) Any such transform has to reason about all the uses, so the llvm.objectsize call itself would prevent the transform from happening. https://github.com/llvm/llvm-project/pull/78526 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] [Clang] Correct __builtin_dynamic_object_size for subobject types (PR #78526)
zygoloid wrote: > My answer for the question "what's the semantics of GCC's builtin X?" has > always been "whatever GCC does." It's the best we can rely upon. But then we > get into situations like this, where you and @nikic have one interpretation > of their documentation and I have another. I can point to their behavior to > back up my claim, but in the end it's probably not exactly clear even to GCC. [@nikic demonstrated](https://github.com/llvm/llvm-project/pull/78526#issuecomment-1900439850) that our current behavior is already compatible with GCC's behavior. If GCC's behavior is the spec, then we are allowed to return 48 rather than only 40 or -1 (or presumably 0 if `argc` is out of bounds) for the original example, because in some cases GCC does so. > My concern is that we want to use this for code hardening. Without precise > object sizes, we're hampered in our goal. The unfortunate reality is that we > can only get that size via these `__builtin_[dynamic_]object_size` functions. That's a totally understandable desire, but I think it's not realistic to expect precise *sub*object sizes in the same cases that GCC can provide them, due to the different architectural choices in the two compilers. If we had a mid-level IR for Clang that still had frontend information, we could do better by evaluating BOS there, so maybe that's one long term path forward to consider. And in the short term, while there are cases where we won't be able to match GCC, I think Clang should do better than it currently does in the frontend, specifically in cases like the one in the bug report where there's an obvious better answer that doesn't require any sophisticated analysis to discover. > > Here, `f` ideally would return 4, but at the LLVM IR level, `p` and `q` are > > identical values and the `>a` operation is a no-op. In cases like this, > > the best we can realistically do is to return 8. > > The sub-object for `>a` and even `>b` is `struct X`, not the integers > themselves. If you want that, you'll have to use casts: `&((char > *)p->b)[2];`. (I had to take care to get that correct.) So `f` should return > `8` (note it's likely to get `8` from the `alloc_size` attribute on `malloc` > in your example). GCC disagrees with you: https://godbolt.org/z/s4P74oEqx https://github.com/llvm/llvm-project/pull/78526 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] [Clang] Correct __builtin_dynamic_object_size for subobject types (PR #78526)
zygoloid wrote: > > But mode 0 and 1 are in general asking for an upper bound on the accessible > > bytes (that is, an N so any.access beyond N bytes is definitely out of > > bounds), so it seems to me that returning -1 is strictly worse than > > returning 48. > > It's the second bit that controls whether it's an upper bound or lower bound > that's returned, not the least significant bit. Right, that's what I said: modes 0 and 1 ask for an upper bound. (Modes 2 and 3 ask for a lower bound.) > We're trying to implement a GNU builtin, and the only defined semantics we > have to go on are GNU's documentation. I can't see how we can deviate from > their documentation unless it's to say "we can't determine this value" and so > return `-1` instead of an answer that might be wildly wrong and potentially > cause a memory leak of some sort. We're not really deviating from the documented rules. By the time we get to LLVM IR lowering of the builtin, we have lost the precise frontend information. But we know the pointer might point into the complete object (where it would be able to write to 48 bytes) or to some subobject of that complete object (where it would be able to write to 48 bytes or less). Therefore the correct answer to return is 48. In the same way, it would be correct to return 48 here: ``` char a[48]; char b[40]; bool always_false = false; // happens to never be modified int n = __builtin_dynamic_object_size(always_false ? a : b, 1); ``` ... though we miscompile this and return 40 regardless of whether we're in upper bound or lower bound mode. :-( > In my made-up example, if we said, "Yes you can write up to 48 bytes into > `p->failed_devs[argc]`, then a user may overwrite the two fields after > `field_devs`. If we return `-1`, they'll have to take the "slow", but ideally > more secure, route. No, that is not what we are saying when we return 48. We're saying "a write past 48 bytes is definitely bad". If you want a lower bound on the number of bytes that you can write, then you need to use mode 2 or 3 instead. https://github.com/llvm/llvm-project/pull/78526 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] [Clang] Correct __builtin_dynamic_object_size for subobject types (PR #78526)
bwendling wrote:
> @bwendling I think you are reading the GCC docs too pedantically. In
> particular, they also say
> > If there are multiple objects ptr can point to and all of them are known at
> > compile time, the returned number is the maximum of remaining byte counts
> > in those objects if type & 2 is 0 and minimum if nonzero.
Isn't this referring to pointing to an object in a union? That's only way this
makes sense (in C).
> which makes it abundantly clear that what you get is an upper bound or lower
> bound, respectively. -1 and 0 are just the upper and lower bounds if you have
> no useful information at all. If you want to check whether the size is
> _exactly_ known, you'll have to compare both bounds. Outside of doing that,
> you can never assume that the bound is precise.
I'm still not convinced that it's abundantly clear... but if we want to compare
max vs. min, things get worse because we return `0` when the `type` is `3`
(https://godbolt.org/z/4f5onM3W7). So even with checking upper and lower
bounds, we come away knowing nothing about the size of the object (probably a
deficiency in Clang).
> Btw, it looks like your initial example gets 48 for both modes on GCC as
> well? https://c.godbolt.org/z/EfGWv4Wrh
If you remove the `argc = 1;`, you get the difference. Your example might be a
GCC bug.
> > All of these are explicit in the LLVM IR. Is the worry that they've been
> > changed from some transformations? Or are there others I'm missing?
>
> Apart from the fact that what you are doing is simply illegal under our IR
> semantics, a practical case where this will likely compute incorrect results
> are unions.
?? I think it's a bit much to say that this is "illegal." We may not get every
instance of something, but we can certainly determine the size of a struct. If
by "illegal" you mean determining what a sub-object is, I don't agree but if
it's so there might be a way to retain whatever information is needed to
determine what a sub-object is.
> For unions, clang will use the type of the union member with the largest size
> as the alloca type, regardless of which union member is active. I haven't
> tried, but your patch will probably compute the subobject size based on that
> arbitrarily picked member, rather than the one being accessed.
We don't seem to support unions correctly now:
```
$ cat ~/llvm/__bdos3.c
struct suspend_stats {
int failed_resume_noirq;
int last_failed_dev;
union {
charfailed_devs[2][40];
charfailed_devs2[38];
};
int last_failed_errno;
int bar;
};
#define report(x) __builtin_printf(#x ": %zu\n", x)
int main(int argc, char *argv[])
{
struct suspend_stats foo;
report(__builtin_dynamic_object_size(_devs2[argc], 0));
report(__builtin_dynamic_object_size(_devs2[argc], 1));
report(__builtin_dynamic_object_size(_devs2[argc], 2));
report(__builtin_dynamic_object_size(_devs2[argc], 3));
return 0;
}
$ clang -O2 ~/llvm/__bdos3.c && ./a.out
__builtin_dynamic_object_size(_devs2[argc], 0): 87
__builtin_dynamic_object_size(_devs2[argc], 1): 87
__builtin_dynamic_object_size(_devs2[argc], 2): 87
__builtin_dynamic_object_size(_devs2[argc], 3): 0
$ gcc -O2 ~/llvm/__bdos3.c && ./a.out
__builtin_dynamic_object_size(_devs2[argc], 0): 87
__builtin_dynamic_object_size(_devs2[argc], 1): 37
__builtin_dynamic_object_size(_devs2[argc], 2): 87
__builtin_dynamic_object_size(_devs2[argc], 3): 37
```
https://github.com/llvm/llvm-project/pull/78526
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] [Clang] Correct __builtin_dynamic_object_size for subobject types (PR #78526)
nikic wrote: @bwendling I think you are reading the GCC docs too pedantically. In particular, they also say > If there are multiple objects ptr can point to and all of them are known at > compile time, the returned number is the maximum of remaining byte counts in > those objects if type & 2 is 0 and minimum if nonzero. which makes it abundantly clear that what you get is an upper bound or lower bound, respectively. -1 and 0 are just the upper and lower bounds if you have no useful information at all. If you want to check whether the size is *exactly* known, you'll have to compare both bounds. Outside of doing that, you can never assume that the bound is precise. Btw, it looks like your initial example gets 48 for both modes on GCC as well? https://c.godbolt.org/z/EfGWv4Wrh > All of these are explicit in the LLVM IR. Is the worry that they've been > changed from some transformations? Or are there others I'm missing? Apart from the fact that what you are doing is simply illegal under our IR semantics, a practical case where this will likely compute incorrect results are unions. For unions, clang will use the type of the union member with the largest size as the alloca type, regardless of which union member is active. I haven't tried, but your patch will probably compute the subobject size based on that arbitrarily picked member, rather than the one being accessed. https://github.com/llvm/llvm-project/pull/78526 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] [Clang] Correct __builtin_dynamic_object_size for subobject types (PR #78526)
zygoloid wrote:
> When you say that we can't detect what the front-end considers the "closest
> surrounding subobject" to be, is that mostly due to corner cases or is it a
> more general concern?
It's a more general concern: LLVM simply has no idea what the frontend
considers to be a subobject. The LLVM type doesn't carry that information.
> ```
> struct suspend_stats {
> //...
> charfailed_devs[REC_FAILED_NUM][40];
> int last_failed_errno;
> int bar;
> };
> //...
> ```
>
> Without the change, the last line is:
>
> ```
> __builtin_dynamic_object_size(foo.failed_devs[argc], 1): 48
> ```
>
> Which isn't correct according to GNU's documentation. So if we can't honor
> the TYPE bit, then we should return `-1 / 0` here, right?
Perhaps according to the GCC documentation as written. But mode 0 and 1 are in
general asking for an upper bound on the accessible bytes (that is, an N so
any.access beyond N bytes is definitely out of bounds), so it seems to me that
returning -1 is strictly worse than returning 48. Do you have a use case for
which -1 is a better answer?
I suspect the only change we're missing here is a change to our documentation
to explicitly say that we give an upper/lower bound when we can't compute an
exact size.
https://github.com/llvm/llvm-project/pull/78526
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] [Clang] Correct __builtin_dynamic_object_size for subobject types (PR #78526)
https://github.com/nikic requested changes to this pull request. Using anything but the size and alignment of the alloca type in a way that affects program semantics is illegal. https://github.com/llvm/llvm-project/pull/78526 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
