Incorrect stack coloring for alloca in SEH cleanuppad #66984
Comments
|
Failed to cherry-pick: a47fa1ea35e5fe37c7e36b91ef172ad237f56a65 https://github.com/llvm/llvm-project/actions/runs/6273163713 Please manually backport the fix and push it to your github fork. Once this is done, please add a comment like this:
|
|
/branch llvm/llvm-project-release-prs/issue66984 |
llvmbot
pushed a commit
to llvm/llvm-project-release-prs
that referenced
this issue
Sep 22, 2023
The write to the SEH catch object happens before cleanuppads are executed, while the first reference to the object will typically be in a catchpad. If we make use of first-use analysis, we may end up allocating an alloca used inside the cleanuppad and the catch object at the same stack offset, which would be incorrect. https://reviews.llvm.org/D86673 was a previous attempt to fix it. It used the heuristic "a slot loaded in a WinEH pad and never written" to detect catch objects. However, because it checks for more than one load (while probably more than zero was intended), the fix does not actually work. The general approach also seems dubious to me, so this patch reverts that change entirely, and instead marks all catch object slots as conservative (i.e. excluded from first-use analysis) based on the WinEHFuncInfo. As far as I can tell we don't need any heuristics here, we know exactly which slots are affected. Fixes llvm/llvm-project#66984. (cherry picked from commit b3cb4f069c2cb99bdae68d6906156af20e76314f)
|
/pull-request llvm/llvm-project-release-prs#706 |
tru
pushed a commit
to llvm/llvm-project-release-prs
that referenced
this issue
Sep 27, 2023
The write to the SEH catch object happens before cleanuppads are executed, while the first reference to the object will typically be in a catchpad. If we make use of first-use analysis, we may end up allocating an alloca used inside the cleanuppad and the catch object at the same stack offset, which would be incorrect. https://reviews.llvm.org/D86673 was a previous attempt to fix it. It used the heuristic "a slot loaded in a WinEH pad and never written" to detect catch objects. However, because it checks for more than one load (while probably more than zero was intended), the fix does not actually work. The general approach also seems dubious to me, so this patch reverts that change entirely, and instead marks all catch object slots as conservative (i.e. excluded from first-use analysis) based on the WinEHFuncInfo. As far as I can tell we don't need any heuristics here, we know exactly which slots are affected. Fixes llvm/llvm-project#66984. (cherry picked from commit b3cb4f069c2cb99bdae68d6906156af20e76314f)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Consider the following example (https://llvm.godbolt.org/z/h1WovoPMG):
Here
%a1has a lifetime fully contained within thecleanuppad, while%a2has a lifetime across the whole function. However, the first use of%a2is inside thecatchpad.%a2here is the pointer into which the exception object for the catchpad gets written.With
stackcoloring-lifetime-start-on-first-use(the default),%a1and%a2are allocated to the same stack slot. On the surface, this looks fine (because%a2is only used after the cleanuppad). However, there is a quirk of SEH exception handling that makes this incorrect: The exception will be copied into%a2by the personality function before both thecleanuppadandcatchpadare called. As such, the write to%a2happens earlier than our IR modeling implies.There has been a patch to fix this issue in https://reviews.llvm.org/D86673. Unfortunately, that patch does not handle this case, because it checks for
NumLoadInCatchPad[slot] > 1-- I think the intention might have been to write> 0. (Funnily, just completely undoing the changes from that patch doesn't make the patch test case fail...)The text was updated successfully, but these errors were encountered: