Page MenuHomePhabricator
Differential D144232

[PowerPC] Correctly use ELFv2 ABI on FreeBSD/powerpc64
ClosedPublic
Actions

Authored by pkubaj on Feb 16 2023, 3:59 PM.

Details

Reviewers
dim
nemanjai
adalava
Summary

Currently ELFv2 is mostly set, but some places still default to ELFv1.
While this mostly works, it breaks in particular when lld is used with LTO,
as reported in https://github.com/llvm/llvm-project/issues/46697.

Diff Detail

Unit TestsFailed
View All

TimeTest
60,910 msx64 debian > Clang.Driver::fsanitize.c
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang --target=x86_64-linux-gnu -fsanitize=undefined -fsanitize-trap=undefined /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/fsanitize.c -### 2>&1 | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/fsanitize.c --check-prefix=CHECK-UNDEFINED-TRAP
510 msx64 debian > LLVM.CodeGen/PowerPC::ppc64-elf-abi.ll
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc -verify-machineinstrs -mtriple=powerpc64-unknown-linux-gnu < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/PowerPC/ppc64-elf-abi.ll | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/PowerPC/ppc64-elf-abi.ll -check-prefix=CHECK-ELFv1
100 msx64 debian > LLVM.CodeGen/PowerPC::pr47373.ll
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc -mtriple=powerpc64-unknown-freebsd13.0 -verify-machineinstrs -mcpu=ppc64 -ppc-asm-full-reg-names < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/PowerPC/pr47373.ll | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/PowerPC/pr47373.ll

Event Timeline

pkubaj created this revision.Feb 16 2023, 3:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 16 2023, 3:59 PM
Herald added subscribers: shchenz, kbarton, hiraditya. · View Herald Transcript
pkubaj requested review of this revision.Feb 16 2023, 3:59 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 16 2023, 3:59 PM
Herald added subscribers: llvm-commits, cfe-commits. · View Herald Transcript
pkubaj updated this revision to Diff 498185.Feb 16 2023, 4:02 PM

Fix indenting according to clang-format suggestions.

Harbormaster completed remote builds in B214289: Diff 498185.Feb 16 2023, 5:52 PM
adalava added a comment.Feb 16 2023, 7:04 PM

Nice catch!
I'm not a LLVM maintainer but from FreeBSD PowerPC side I agree with the changes, only added two suggestions that could improve code readability

clang/lib/Basic/Targets/PPC.h
428

The comparison Triple.getOSMajorVersion() == 0 works, but I think Triple.getOSVersion().empty() would better describe the intent

llvm/lib/Target/PowerPC/PPCTargetMachine.cpp
237

Same here, it could be TT.getOSVersion().empty() instead of TT.getOSMajorVersion() == 0

adalava accepted this revision.Feb 16 2023, 7:04 PM
This revision is now accepted and ready to land.Feb 16 2023, 7:04 PM
pkubaj updated this revision to Diff 498378.Feb 17 2023, 7:58 AM

Addres adalava's review.

Harbormaster completed remote builds in B214420: Diff 498378.Feb 17 2023, 8:49 AM
brad mentioned this in D144321: [PowerPC] Correctly use ELFv2 ABI on all OS's that use the ELFv2 ABI.Feb 17 2023, 10:44 PM
brad added a subscriber: brad.

I noticed this review. I have provided a more complete diff for review at D144321.

dim added a comment.Feb 18 2023, 4:22 PM
In D144232#4136787, @brad wrote:

I noticed this review. I have provided a more complete diff for review at D144321.

Yeah I think that is probably the better option, hope @pkubaj and @adalava agree with that?

As Brad's version covers both FreeBSD and OpenBSD, and also updates a bunch of unit tests, which this review appears to break (see the Unit Tests https://reviews.llvm.org/harbormaster/unit/214420/).

adalava added a comment.Feb 27 2023, 10:45 AM
In D144232#4137367, @dim wrote:
In D144232#4136787, @brad wrote:

I noticed this review. I have provided a more complete diff for review at D144321.

Yeah I think that is probably the better option, hope @pkubaj and @adalava agree with that?

As Brad's version covers both FreeBSD and OpenBSD, and also updates a bunch of unit tests, which this review appears to break (see the Unit Tests https://reviews.llvm.org/harbormaster/unit/214420/).

I agree with D144321, sorry for the late reply. @pkubaj and @brad , thanks for pushing it. I think D144232 can be abandoned, right?

brad added a comment.Feb 27 2023, 12:03 PM
In D144232#4155966, @adalava wrote:

I agree with D144321, sorry for the late reply. @pkubaj and @brad , thanks for pushing it. I think D144232 can be abandoned, right?

Correct.

dim closed this revision.Feb 27 2023, 12:03 PM

Revision Contents
Changeset List

PathSize
clang/
lib/
Basic/
Targets/
4 lines
llvm/
lib/
Target/
PowerPC/
5 lines

Diff 498378

View Options

clang/lib/Basic/Targets/PPC.h

Loading...
View Options

llvm/lib/Target/PowerPC/PPCTargetMachine.cpp

Loading...