vsoch created this revision.
vsoch added reviewers: LLVM, llvm.org.
vsoch created this object with edit policy "Only User: vsoch (Vanessasaurus)".
vsoch added a project: LLVM.
Herald added a subscriber: pengfei.
vsoch requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
The System V ABI standard (page 22 point 5.a
https://raw.githubusercontent.com/dyninst/ABI-Analysis-for-Dynamic-Calls/master/callsite_parsing/docs/AMD64/x86-64-psABI-1.0.pdf)
states that:
> If one of the classes is MEMORY, the whole argument is passed in memory
F19433717: image.png <https://reviews.llvm.org/F19433717>
Implying that both classes Lo and Hi should be checked. However the clang
matching code does not honor this request, as it only checks if Hi == Memory
(and then updates Lo). Reading the standard (which is also included in the
docstring) it can take a developer down a rabbit hole worrying about the lack
of consistency. In basic testing of a struct with large values
(https://github.com/buildsi/llvm-bug/tree/main/struct), the result I found was
that the postMerge step would return MEMORY NOCLASS without this fix, and
MEMORY MEMORY with it. Since if either both classes are MEMORY or one is MEMORY
and the other NOCLASS both results in MEMORY, this would not be an ABI break
(the resulting binaries are the same), however I cannot attest that there
aren't other niche cases that might lead to a break. It also could be the case
that a developer is using this function for a different use case, and then
would get a different result. For these reasons, and possibly saving someone
future time or angst to see a clear difference between what the standard says
and the implementation, I am suggesting a fix to the postMerge function so that
it properly reflects the System V ABI document to check both Lo and Hi.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D111178
Files:
clang/lib/CodeGen/TargetInfo.cpp
Index: clang/lib/CodeGen/TargetInfo.cpp
===================================================================
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -2776,8 +2776,10 @@
//
// Note that clauses (b) and (c) were added in 0.98.
//
- if (Hi == Memory)
+ if ((Hi == Memory) || (Lo == Memory)) {
Lo = Memory;
+ Hi = Memory;
+ }
if (Hi == X87Up && Lo != X87 && honorsRevision0_98())
Lo = Memory;
if (AggregateSize > 128 && (Lo != SSE || Hi != SSEUp))
Index: clang/lib/CodeGen/TargetInfo.cpp
===================================================================
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -2776,8 +2776,10 @@
//
// Note that clauses (b) and (c) were added in 0.98.
//
- if (Hi == Memory)
+ if ((Hi == Memory) || (Lo == Memory)) {
Lo = Memory;
+ Hi = Memory;
+ }
if (Hi == X87Up && Lo != X87 && honorsRevision0_98())
Lo = Memory;
if (AggregateSize > 128 && (Lo != SSE || Hi != SSEUp))
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
