This is an archive of the discontinued LLVM Phabricator instance.

[Clang][AIX][p] Manually claim -p in front end
ClosedPublic

Authored by francii on Feb 28 2023, 5:00 PM.

Details

Summary

The current implementation of -p does not claim the argument once it is passed. Since it pushes -pg directly, it is only ever referred to again when linking. As a result, when compiling with -S, the compiler warns that -p goes unused even though that is not the case.

With this patch, if both -p and -pg are passed, the argument that is passed second will take precedence. -p will still throw an error on unsupported platforms, regardless of precedence.

This revision includes a test case, which has been placed in clang/test/Driver/zos-profiling-error.c. As a result, zos-profiling-error.c has been renamed to ibm-profiling.c. This revision also passes clang/test/Driver/aix-ld.c.

Diff Detail

Event Timeline

francii created this revision.Feb 28 2023, 5:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2023, 5:00 PM
francii requested review of this revision.Feb 28 2023, 5:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2023, 5:00 PM
francii edited the summary of this revision. (Show Details)Feb 28 2023, 5:01 PM
francii added a reviewer: daltenty.
francii updated this revision to Diff 501526.Mar 1 2023, 8:35 AM

Add test case

francii updated this revision to Diff 501527.Mar 1 2023, 8:41 AM

Merge z/OS and AIX profiling tests

francii updated this revision to Diff 501529.Mar 1 2023, 8:43 AM

Remove old test case

francii updated this revision to Diff 501530.Mar 1 2023, 8:44 AM

Remove old test case

francii updated this revision to Diff 501531.Mar 1 2023, 8:46 AM

Add test case descriptions

francii edited the summary of this revision. (Show Details)Mar 1 2023, 8:53 AM
francii edited the summary of this revision. (Show Details)
daltenty accepted this revision.Mar 1 2023, 8:55 AM

LGTM, thanks!

This revision is now accepted and ready to land.Mar 1 2023, 8:55 AM
daltenty added inline comments.Mar 1 2023, 8:57 AM
clang/lib/Driver/ToolChains/Clang.cpp
6327

Actually, a question here. Does this now result in the arg being claimed even if nothing was done it? Since there are case we don't go into the error path.

francii updated this revision to Diff 501666.Mar 1 2023, 2:23 PM

Update to claiming logic:
if -pg is passed, only claim -p if -p is passed afterwards.
Otherwise, always claim -p.

francii edited the summary of this revision. (Show Details)Mar 1 2023, 2:45 PM
francii edited the summary of this revision. (Show Details)
francii edited the summary of this revision. (Show Details)
francii edited the summary of this revision. (Show Details)Mar 1 2023, 2:47 PM
francii edited the summary of this revision. (Show Details)Mar 1 2023, 2:59 PM
francii edited the summary of this revision. (Show Details)Mar 1 2023, 4:00 PM
francii updated this revision to Diff 501692.Mar 1 2023, 4:04 PM

Always throw error when -p is in command.

francii updated this revision to Diff 501698.Mar 1 2023, 4:39 PM

Add more test cases

MaskRay added inline comments.Mar 1 2023, 9:37 PM
clang/test/Driver/ibm-profiling.c
2

This is too verbose. Driver tests prefer packing many options on one line. The decreased number of lines actually improves readability.

MaskRay added inline comments.Mar 1 2023, 9:39 PM
clang/lib/Driver/ToolChains/Clang.cpp
6347

Use &&

francii updated this revision to Diff 501929.Mar 2 2023, 11:03 AM

Update based on review

francii marked 3 inline comments as done.Mar 2 2023, 11:04 AM
francii added inline comments.Mar 3 2023, 11:30 AM
clang/lib/Driver/ToolChains/Clang.cpp
6327

I've updated the logic of this patch entirely. With these changes, using -p and -pg together now gives precedence to the option that is passed last.

Now, whenever we check for -p, we never do a blind claim. We instead manually claim -p in the following instances:

  1. The user passes -p without also passing -pg: We manually claim -p, and push -pg to CmdArgs.
  2. The user passes both -p and -pg, but -p is given precedence: We manually claim -p, and do nothing (since -pg is already in CmdArgs).

As a result, -p will only warn that it goes unused if it is passed before -pg (i.e. clang -p -pg).

francii updated this revision to Diff 502204.Mar 3 2023, 11:35 AM

Fix missing bracket

francii updated this revision to Diff 503109.Mar 7 2023, 11:24 AM

Test case nit

francii retitled this revision from [Clang][AIX][p] Claim -p in front end to [Clang][AIX][p] Manually claim -p in front end.Mar 11 2023, 11:32 PM
This revision was landed with ongoing or failed builds.Mar 11 2023, 11:34 PM
This revision was automatically updated to reflect the committed changes.
francii reopened this revision.Mar 14 2023, 11:11 AM
This revision is now accepted and ready to land.Mar 14 2023, 11:11 AM

Reopen since this was reverted

francii updated this revision to Diff 505186.Mar 14 2023, 11:12 AM

Make -p a CC1 option.

francii updated this revision to Diff 505560.Mar 15 2023, 10:34 AM

Fix segfault

francii updated this revision to Diff 505601.Mar 15 2023, 1:14 PM

Don't claim when checking for object file

francii updated this revision to Diff 505653.Mar 15 2023, 4:27 PM

Rebase for push

This revision was landed with ongoing or failed builds.Mar 15 2023, 4:28 PM
This revision was automatically updated to reflect the committed changes.
clang/lib/Driver/ToolChains/Clang.cpp