This is an archive of the discontinued LLVM Phabricator instance.

Support -fclang-abi-compat=8.0 to keep old ABI behavior
Needs ReviewPublic

Authored by wxiao3 on Jun 17 2019, 8:49 PM.

Details

Summary

The System V i386 bug fix (https://reviews.llvm.org/D59744) makes it impossible
for 32-bit Linux Chromium to write an assembly function that works with both
trunk clang and clang 8.0.0, which makes it difficult to update compilers
independent of changing the code (more details:
https://bugs.chromium.org/p/chromium/issues/detail?id=974542#c5).

This patch aims to provide a solution for such situation.

Diff Detail

Event Timeline

wxiao3 created this revision.Jun 17 2019, 8:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2019, 8:49 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
compnerd added a subscriber: compnerd.EditedJun 17 2019, 9:08 PM

Could you please add a test to ensure that Darwin defaults to the old behaviour?

include/clang/Basic/LangOptions.h
142

Referencing the SVN revision is better IMO as the infrastructure can change, but, the reference to the commit is useful as that can be mapped.

wxiao3 updated this revision to Diff 205261.Jun 17 2019, 11:57 PM
wxiao3 added a reviewer: compnerd.
wxiao3 marked an inline comment as done.

Yes, there is a test to ensure that Darwin defaults to the old behaviour in "test/CodeGen/x86_32-m64.c".

RKSimon added inline comments.Jun 18 2019, 12:59 AM
test/CodeGen/x86_32-m64.c
6

Use ABI80 instead of OLDABI?

hans added a comment.Jun 18 2019, 1:59 AM

The System V i386 bug fix (https://reviews.llvm.org/D59744) makes it impossible

Please refer to the svn revision instead of the code review.

for 32-bit Linux Chromium to write an assembly function that works with both
trunk clang and clang 8.0.0, which makes it difficult to update compilers
independent of changing the code (more details:
https://bugs.chromium.org/p/chromium/issues/detail?id=974542#c5).

We still don't understand the Chromium problem so we should probably wait until that's done, and then the commit message should be more general. (For Chromium, we will not want to lock to an old ABI version but rather do whatever fixing is required.)

This patch aims to provide a solution for such situation.

docs/ReleaseNotes.rst
146

Instead of "historically", please spell out exactly what versions, i.e. versions prior to Clang 9.

151

Just refer to the major version: `-fclang-abi-compat=8`

include/clang/Basic/LangOptions.h
142

Isn't the comment backwards? Setting Ver8 causes __m64 *not* to be passed in an MMX register?

lib/CodeGen/TargetInfo.cpp
1091

What about Clang 8.0.1? :-) Probably better to say "Clang 8 and previous versions did not do this."

test/CodeGen/x86_32-m64.c
6

Please pass -fclang-abi-compat=8 instead to match how the flag is used otherwhere.

krytarowski resigned from this revision.Oct 10 2020, 3:17 AM
thakis resigned from this revision.Oct 16 2020, 11:29 AM

hans and compnerd got this, no need for so many reviewers :)