This is an archive of the discontinued LLVM Phabricator instance.

Fix clang postMerge logic based on System V ABI Standard
Needs ReviewPublic

Authored by vsoch on Oct 5 2021, 12:28 PM.

Details

Reviewers
llvm.org
Group Reviewers
Restricted Project
Summary

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

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.

Diff Detail

Event Timeline

vsoch created this revision.Oct 5 2021, 12:28 PM
vsoch created this object with edit policy "vsoch (Vanessasaurus)".
vsoch requested review of this revision.Oct 5 2021, 12:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 5 2021, 12:28 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Testcase? Can this patch fix the problem in D107965?