[PATCH] D111178: Fix clang postMerge logic based on System V ABI Standard

[Vanessasaurus via Phabricator via cfe-commits]

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))


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D111178.377329.patch
Type: text/x-patch
Size: 511 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20211005/8bc9999d/attachment-0001.bin>