LLVM is growing knowledge of this, so we should inform it when we have
the information available. As a bonus, since we now check return
alignments in the callee as of an earlier patch of mine, we can now drop
the redundant llvm.assume() that this previously emitted.
Details
Details
- Reviewers
jyknight lebedev.ri
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| clang/test/CodeGen/alloc-align-attr.c | ||
|---|---|---|
| 11–12 | now this llvm.assume is redudant and should be removed? | |
| clang/test/CodeGen/alloc-align-attr.c | ||
|---|---|---|
| 11–12 | I don't think so? Right now allocalign doesn't mean a whole lot (it really just answers some questions that used to be inferred from function name by MemoryBuiltins.cpp) so I'd rather leave the llvm.assume given that I doubt it's hurting anyone? | |
| clang/lib/CodeGen/CGCall.cpp | ||
|---|---|---|
| 4880 | We do need to fallback to an assume for "if(CGF.SanOpts.has(SanitizerKind::Alignment)" however -- and NOT emit an allocalign (nor an align) attribute in that case. This is necessary so that clang can emit explicit misalignment checks FIRST (and abort with a message if they fail), before letting LLVM assume the specified alignment. In order for the ubsan alignment check to work properly, we need to not trigger misalignment-UB before the check executes! See clang/test/CodeGen/catch-alignment-assumption-attribute-assume_aligned-on-function.cpp -- note how in the sanitized mode, it doesn't have an the "align 128" on the call result, but instead emits a bunch of code to test alignment, and branch to an ubsan abort on failure -- followed by the llvm.assume only on the successful branch. Although the test-case is only testing assume_aligned, the same behavior should be applicable to allocalign. (Which shows that we clearly need a sanitized allocalign test-case, too) | |
| clang/test/CodeGen/alloc-align-attr.c | ||
| 11–12 | I think we should get rid of the assume -- it's redundant. If we're emitting an allocalign attribute, I think we shouldn't bother emitting an align attribute, nor an assume. The prior code either adds an "align" to the return value, or an assume instruction afterwards. Both of those are redundant with allocalign: the former will already be done automatically on the llvm side when the LLVM side sees a constant argument for an allocalign parameter, and the latter doesn't provide any more information than the allocalign itself does. | |
| clang/lib/CodeGen/CGCall.cpp | ||
|---|---|---|
| 4880 | It sounds like (between this comment and the one below) I should just abandon the code-sharing between AllocAlignAttrEmitter and AssumeAlignedAttrEmitter - is that right? Also, should I be making a new test case or adding to an existing one for a sanitized allocalign test case? Is that an llvm-level thing or a clang-level thing? | |
| clang/lib/CodeGen/CGCall.cpp | ||
|---|---|---|
| 4880 | Agree, it does sound like there's not going to be much code-sharing, since emitAlignmentAssumption is already separate. If you find it's clearer to make them be separate, that sounds reasonable. Yes, there should be a test-case for sanitized allocalign, just like for sanitized assume_align. | |
| clang/test/CodeGen/catch-alignment-assumption-attribute-alloc_align-on-function-variable.cpp | ||
|---|---|---|
| 97–98 | This is a regression, the old behavior was correct. | |
| clang/test/CodeGen/catch-alignment-assumption-attribute-alloc_align-on-function-variable.cpp | ||
|---|---|---|
| 97–98 | I don't think so: we now (as of D121629) check the alignment of the returned pointer in the callee instead of the caller, and LLVM knows about allocalign implying an alignment at the callsite. So I think it's correctly optimizing away the sanitizer checks here. You or @jyknight will need to correct me if my understanding is incorrect here, but my understanding was that this change was what motivated D121629 in the first place. | |
| clang/test/CodeGen/catch-alignment-assumption-attribute-alloc_align-on-function-variable.cpp | ||
|---|---|---|
| 97–98 | I'm saying that as the author of said test and it's behavior. | |
| clang/test/CodeGen/catch-alignment-assumption-attribute-alloc_align-on-function-variable.cpp | ||
|---|---|---|
| 97–98 | How is this wrong though? Are you concerned because the allocalign isn't being asserted upon by the sanitizer in the caller? How this a problem given that the callee code now does the assertions in sanitizer mode? (If I'm not understanding please elaborate: this is my first nontrivial work on LLVM, so I'm going to need more than "I wrote this, and you are wrong" by way of help understanding my error.) | |
| clang/test/CodeGen/catch-alignment-assumption-attribute-alloc_align-on-function-variable.cpp | ||
|---|---|---|
| 97–98 | The original code was not relying on the happenstance that the called code would have been | |
| clang/test/CodeGen/catch-alignment-assumption-attribute-alloc_align-on-function-variable.cpp | ||
|---|---|---|
| 97–98 | I suggested this change, which I think reasonable given that it's the same thing we are doing for null sanitization. E.g. clang -fsanitize=null -emit-llvm -O2 -o - -S /tmp/test.cc on: int &foo(int *x);
int bar(int*x) {
return foo(x);
}-> define dso_local noundef i32 @_Z3barPi(i32* noundef %x) local_unnamed_addr #0 {
entry:
%call = tail call noundef nonnull align 4 dereferenceable(4) i32* @_Z3fooPi(i32* noundef %x)
%0 = load i32, i32* %call, align 4, !tbaa !3
ret i32 %0
}There, too, we validate it in the callee before returning. | |
| clang/test/CodeGen/catch-alignment-assumption-attribute-alloc_align-on-function-variable.cpp | ||
|---|---|---|
| 97–98 | That may be your view on the situation, but i'm not sure i agree with it. | |
Comment Actions
I have some reworking to do on this after my latest round of llvm-side work that I'll try and get out later this week.
Comment Actions
This review may be stuck/dead, consider abandoning if no longer relevant.
Removing myself as reviewer in attempt to clean dashboard.
We do need to fallback to an assume for "if(CGF.SanOpts.has(SanitizerKind::Alignment)" however -- and NOT emit an allocalign (nor an align) attribute in that case.
This is necessary so that clang can emit explicit misalignment checks FIRST (and abort with a message if they fail), before letting LLVM assume the specified alignment. In order for the ubsan alignment check to work properly, we need to not trigger misalignment-UB before the check executes!
See clang/test/CodeGen/catch-alignment-assumption-attribute-assume_aligned-on-function.cpp -- note how in the sanitized mode, it doesn't have an the "align 128" on the call result, but instead emits a bunch of code to test alignment, and branch to an ubsan abort on failure -- followed by the llvm.assume only on the successful branch. Although the test-case is only testing assume_aligned, the same behavior should be applicable to allocalign.
(Which shows that we clearly need a sanitized allocalign test-case, too)