Page MenuHomePhabricator

[Clang][AIX][p]Enable -p Functionality
AcceptedPublic

Authored by francii on Nov 9 2022, 5:06 PM.

Details

Summary

This patch enables -p functionality into Clang on AIX and Linux
To create parity with GCC.

The purpose of the -p flag is similar to that of -pg, but the
results are analyzed with the prof tool as opposed to the gprof tool.
More details can be found in this RFC post:
https://discourse.llvm.org/t/rfc-add-p-driver-support-to-clang/66013?u=francii

On AIX, compiling with -p links against mcrt0.o
and produces a mon.out file analyzed with the prof tool,
while -pg links against gcrt0.o and produces a gmon.outfile
analyzed with the gprof tool. The differences are therefore
only a concern when linking, so calling -p will push -pg to cc1.

An AIX test for -p already exists, and I recently
another test was added here:
https://github.com/llvm/llvm-project/commit/dc9846ce988b9ddfcbc42cd462d5d94b634b3161
As such, there is no AIX test case attached to this patch.

Diff Detail

Event Timeline

francii created this revision.Nov 9 2022, 5:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 9 2022, 5:06 PM
Herald added a subscriber: ormris. · View Herald Transcript
francii requested review of this revision.Nov 9 2022, 5:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 9 2022, 5:06 PM
This comment was removed by francii.
francii edited the summary of this revision. (Show Details)Nov 9 2022, 5:38 PM
francii retitled this revision from [Clang][p]Enable -p Functionality to [Clang][GNU][AIX}[p]Enable -p Functionality.Nov 10 2022, 1:42 PM
francii edited the summary of this revision. (Show Details)
francii edited the summary of this revision. (Show Details)
francii updated this revision to Diff 474604.Nov 10 2022, 1:45 PM

Added Linux implementation and test case

francii retitled this revision from [Clang][GNU][AIX}[p]Enable -p Functionality to [Clang][GNU][AIX][p]Enable -p Functionality.Nov 10 2022, 1:57 PM
francii edited the summary of this revision. (Show Details)Nov 13 2022, 11:01 AM
francii updated this revision to Diff 475009.Nov 13 2022, 11:05 AM

Updated Linux test case
Added profiled library search for AIX.

francii updated this revision to Diff 475196.Nov 14 2022, 10:14 AM

Add profiled libraries check AIX

francii updated this revision to Diff 475242.Nov 14 2022, 12:28 PM

Add sysroot check to aix-ld.c test cases

francii updated this revision to Diff 476203.Nov 17 2022, 11:58 AM

Specify prof and gprof in help text

MaskRay added a comment.EditedNov 17 2022, 1:41 PM

Please make -p accepted for AIX only and don't change the semantics for other targets in this patch. For FreeBSD and Linux (musl and gnu) we can try rejecting -p. If OpenBSD wants to make -p an alias for -pg, that's fine.

Please make -p accepted for AIX only and don't change the semantics for other targets in this patch. For FreeBSD and Linux (musl and gnu) we can try rejecting -p. If OpenBSD wants to make -p an alias for -pg, that's fine.

We can make -p emit a message on Linux while also accepting it as an alias to -pg. Do you have a suggestion as to what that message would be?

Please make -p accepted for AIX only and don't change the semantics for other targets in this patch. For FreeBSD and Linux (musl and gnu) we can try rejecting -p. If OpenBSD wants to make -p an alias for -pg, that's fine.

We can make -p emit a message on Linux while also accepting it as an alias to -pg. Do you have a suggestion as to what that message would be?

The current warning: argument unused during compilation: '-p' [-Wunused-command-line-argument] is good for Linux.
In the future Linux can try removing -p.

Recall that the goal with -p is to create parity with GCC (at least with Linux and AIX), as per the RFC discussion.

Please make -p accepted for AIX only and don't change the semantics for other targets in this patch. For FreeBSD and Linux (musl and gnu) we can try rejecting -p. If OpenBSD wants to make -p an alias for -pg, that's fine.

We can make -p emit a message on Linux while also accepting it as an alias to -pg. Do you have a suggestion as to what that message would be?

The current warning: argument unused during compilation: '-p' [-Wunused-command-line-argument] is good for Linux.
In the future Linux can try removing -p.

The current behaviour of ignoring the option without stopping with an error return code is not a good one.

Recall that the goal is to create parity with GCC, as per the RFC post.

Is there a reason this flag shouldn't be supported on Linux? Specifically, what is your justification for diverging from GCC on this matter?

MaskRay added a comment.EditedNov 17 2022, 5:09 PM

Recall that the goal with -p is to create parity with GCC (at least with Linux and AIX), as per the RFC discussion.

Please make -p accepted for AIX only and don't change the semantics for other targets in this patch. For FreeBSD and Linux (musl and gnu) we can try rejecting -p. If OpenBSD wants to make -p an alias for -pg, that's fine.

We can make -p emit a message on Linux while also accepting it as an alias to -pg. Do you have a suggestion as to what that message would be?

The current warning: argument unused during compilation: '-p' [-Wunused-command-line-argument] is good for Linux.
In the future Linux can try removing -p.

The current behaviour of ignoring the option without stopping with an error return code is not a good one.

Recall that the goal is to create parity with GCC, as per the RFC post.

Is there a reason this flag shouldn't be supported on Linux? Specifically, what is your justification for diverging from GCC on this matter?

It's a legacy option (at least for Linux, FreeBSD, etc) and we don't want the usage to grow. I objected in the RFC, either. Note that the objection is not only from me, also from a Linux distro folk I checked with.

Recall that the goal with -p is to create parity with GCC (at least with Linux and AIX), as per the RFC discussion.

Please make -p accepted for AIX only and don't change the semantics for other targets in this patch. For FreeBSD and Linux (musl and gnu) we can try rejecting -p. If OpenBSD wants to make -p an alias for -pg, that's fine.

We can make -p emit a message on Linux while also accepting it as an alias to -pg. Do you have a suggestion as to what that message would be?

The current warning: argument unused during compilation: '-p' [-Wunused-command-line-argument] is good for Linux.
In the future Linux can try removing -p.

The current behaviour of ignoring the option without stopping with an error return code is not a good one.

Recall that the goal is to create parity with GCC, as per the RFC post.

Is there a reason this flag shouldn't be supported on Linux? Specifically, what is your justification for diverging from GCC on this matter?

It's a legacy option (at least for Linux, FreeBSD, etc) and we don't want the usage to grow. I objected in the RFC, either. Note that the objection is not only from me, also from a Linux distro folk I checked with.

If we aren't adding Linux functionality, we should make it throw an error at the same time.

Once again, ignoring the option without stopping with an error code is not ideal. I can update this patch to throw an error on Linux, much like this patch for z/OS: https://reviews.llvm.org/D137756

Recall that the goal with -p is to create parity with GCC (at least with Linux and AIX), as per the RFC discussion.

Please make -p accepted for AIX only and don't change the semantics for other targets in this patch. For FreeBSD and Linux (musl and gnu) we can try rejecting -p. If OpenBSD wants to make -p an alias for -pg, that's fine.

We can make -p emit a message on Linux while also accepting it as an alias to -pg. Do you have a suggestion as to what that message would be?

The current warning: argument unused during compilation: '-p' [-Wunused-command-line-argument] is good for Linux.
In the future Linux can try removing -p.

The current behaviour of ignoring the option without stopping with an error return code is not a good one.

Recall that the goal is to create parity with GCC, as per the RFC post.

Is there a reason this flag shouldn't be supported on Linux? Specifically, what is your justification for diverging from GCC on this matter?

It's a legacy option (at least for Linux, FreeBSD, etc) and we don't want the usage to grow. I objected in the RFC, either. Note that the objection is not only from me, also from a Linux distro folk I checked with.

If we aren't adding Linux functionality, we should make it throw an error at the same time.

Once again, ignoring the option without stopping with an error code is not ideal. I can update this patch to throw an error on Linux, much like this patch for z/OS: https://reviews.llvm.org/D137756

I acknowledge that the current state is bad. Reject it for FreeBSD/Linux (perhaps most OSes. An OS can opt in if their platform really needs this) is likely fine. I don't think -p has many uses.

Something like D137756 will be nice, but I think it can be done in clang/lib/Driver/ToolChains/Clang.cpp: D138255

Recall that the goal with -p is to create parity with GCC (at least with Linux and AIX), as per the RFC discussion.

Please make -p accepted for AIX only and don't change the semantics for other targets in this patch. For FreeBSD and Linux (musl and gnu) we can try rejecting -p. If OpenBSD wants to make -p an alias for -pg, that's fine.

We can make -p emit a message on Linux while also accepting it as an alias to -pg. Do you have a suggestion as to what that message would be?

The current warning: argument unused during compilation: '-p' [-Wunused-command-line-argument] is good for Linux.
In the future Linux can try removing -p.

The current behaviour of ignoring the option without stopping with an error return code is not a good one.

Recall that the goal is to create parity with GCC, as per the RFC post.

Is there a reason this flag shouldn't be supported on Linux? Specifically, what is your justification for diverging from GCC on this matter?

It's a legacy option (at least for Linux, FreeBSD, etc) and we don't want the usage to grow. I objected in the RFC, either. Note that the objection is not only from me, also from a Linux distro folk I checked with.

If we aren't adding Linux functionality, we should make it throw an error at the same time.

Once again, ignoring the option without stopping with an error code is not ideal.

I can update this patch to throw an error, much like this patch for z/OS: https://reviews.llvm.org/D137756

Recall that the goal with -p is to create parity with GCC (at least with Linux and AIX), as per the RFC discussion.

Please make -p accepted for AIX only and don't change the semantics for other targets in this patch. For FreeBSD and Linux (musl and gnu) we can try rejecting -p. If OpenBSD wants to make -p an alias for -pg, that's fine.

We can make -p emit a message on Linux while also accepting it as an alias to -pg. Do you have a suggestion as to what that message would be?

The current warning: argument unused during compilation: '-p' [-Wunused-command-line-argument] is good for Linux.
In the future Linux can try removing -p.

The current behaviour of ignoring the option without stopping with an error return code is not a good one.

Recall that the goal is to create parity with GCC, as per the RFC post.

Is there a reason this flag shouldn't be supported on Linux? Specifically, what is your justification for diverging from GCC on this matter?

It's a legacy option (at least for Linux, FreeBSD, etc) and we don't want the usage to grow. I objected in the RFC, either. Note that the objection is not only from me, also from a Linux distro folk I checked with.

If we aren't adding Linux functionality, we should make it throw an error at the same time.

Once again, ignoring the option without stopping with an error code is not ideal. I can update this patch to throw an error on Linux, much like this patch for z/OS: https://reviews.llvm.org/D137756

I acknowledge that the current state is bad. Reject it for FreeBSD/Linux (perhaps most OSes. An OS can opt in if their platform really needs this) is likely fine. I don't think -p has many uses.

Something like D137756 will be nice, but I think it can be done in clang/lib/Driver/ToolChains/Clang.cpp: D138255

Thank you, I will remove the Linux/Gnu changes from this patch shortly.

francii updated this revision to Diff 476649.Nov 18 2022, 7:15 PM

Remove Linux changes

francii retitled this revision from [Clang][GNU][AIX][p]Enable -p Functionality to [Clang][AIX][p]Enable -p Functionality.Nov 18 2022, 7:15 PM
francii edited the summary of this revision. (Show Details)
francii updated this revision to Diff 487907.Tue, Jan 10, 11:34 AM

Add supports profiling check

francii updated this revision to Diff 490261.Wed, Jan 18, 11:38 AM

Remove codegen option approach

francii updated this revision to Diff 490638.Thu, Jan 19, 12:53 PM

Specifically check for AIX before pushing pg

francii edited the summary of this revision. (Show Details)Thu, Jan 19, 12:54 PM
daltenty added inline comments.Tue, Jan 24, 10:50 AM
clang/include/clang/Driver/Options.td
4178

Why is this a cc1 option if it's not used in CC1?

clang/lib/Driver/ToolChains/AIX.cpp
274

Maybe I'm a bit confused, I though these options had different handling with respect to the paths? Otherwise, it's simply an alias.

francii updated this revision to Diff 492189.Wed, Jan 25, 11:02 AM

Remove cc1 option

francii added inline comments.Wed, Jan 25, 11:04 AM
clang/lib/Driver/ToolChains/AIX.cpp
274

You may be referring to lines 167-175 of this same file:

auto getCrt0Basename = [&Args, IsArch32Bit] {
  // Enable gprofiling when "-pg" is specified.
  if (Args.hasArg(options::OPT_pg))
    return IsArch32Bit ? "gcrt0.o" : "gcrt0_64.o";
  // Enable profiling when "-p" is specified.
  else if (Args.hasArg(options::OPT_p))
    return IsArch32Bit ? "mcrt0.o" : "mcrt0_64.o";
  else
    return IsArch32Bit ? "crt0.o" : "crt0_64.o";
};

This is the only time we handle -p differently. With both -p and -pg, we want to link the profiled libraries.

daltenty accepted this revision.Wed, Jan 25, 12:41 PM

LGTM, with small nit to address before commit

clang/lib/Driver/ToolChains/Clang.cpp
6334–6340

nit: we can save our selves a query and clarify by writing this as if-elseif

This revision is now accepted and ready to land.Wed, Jan 25, 12:41 PM
francii updated this revision to Diff 493054.Sat, Jan 28, 7:27 PM

Update based on review