This is an archive of the discontinued LLVM Phabricator instance.

[clang][AIX] Fix Overly Strict LTO Option Checking against `data-sections` when `mxcoff-roptr` is in Effect
ClosedPublic

Authored by qiongsiwu1 on Jun 2 2023, 12:15 PM.

Details

Summary

The LTO -mxcoff-roptr check against data sections is overly strict and it ignores the fact that data sections is on by default on AIX, causing valid LTO compilation to fail when -fdata-sections is not explicitly specified.

This patch revises the check so that an error is reported only if data sections is explicitly turned off for LTO.

Diff Detail

Event Timeline

qiongsiwu1 created this revision.Jun 2 2023, 12:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2023, 12:15 PM
qiongsiwu1 requested review of this revision.Jun 2 2023, 12:15 PM
qiongsiwu1 edited the summary of this revision. (Show Details)Jun 2 2023, 12:21 PM

Gentle ping for review. Thanks so much!

MaskRay added inline comments.Jun 6 2023, 7:44 AM
clang/lib/Driver/ToolChains/CommonArgs.cpp
751–752

Consider the hasFlagNoClaim variant. OPT_fdata_sections is mainly claimed elsewhere.

The LTO mxcoff-roptr check against data

-mxcoff-roptr. Consider not dropping the leading -.

qiongsiwu1 edited the summary of this revision. (Show Details)Jun 6 2023, 1:15 PM
qiongsiwu1 edited the summary of this revision. (Show Details)
qiongsiwu1 updated this revision to Diff 529030.Jun 6 2023, 1:44 PM

Address code review. Thanks @MaskRay for the feedback!

qiongsiwu1 marked an inline comment as done.Jun 7 2023, 6:54 AM

Ping for review. Thank you!

qiongsiwu1 updated this revision to Diff 530903.EditedJun 13 2023, 10:18 AM

Revising the logic to pass -data-sections= to libLTO. We now pass -data-sections=1 to libLTO by default unless it is explicitly turned off.

Old behaviour:

  1. Pass -data-sections=1 if the target requires uses of separate sections, or the user explicitly sets -fdata-sections.
  2. Pass -data-sections=0 if the user explicitly specifies -fno-data-sections.
  3. Pass nothing otherwise.

New behaviour:

  1. Pass -data-section=1 if nothing is specified, or if the target requires uses of separate sections, or the user explicitly sets -fdata-sections. (Combining cases 1 and 3 above.)
  2. Pass -data-sections=0 if the user explicitly specifies -fno-data-sections.

I think the new behaviour is safe because libLTO defaults to use data sections when nothing is specified https://github.com/llvm/llvm-project/blob/829ed96b779c10cb023d7e3dbcfad22413979ec4/llvm/lib/LTO/LTOCodeGenerator.cpp#L426.

@hubert.reinterpretcast @MaskRay could you let me know what you think? Thanks so much!

qiongsiwu1 retitled this revision from [clang][AIX] Fix Overly Strick LTO Option Checking against `data-sections` when `mxcoff-roptr` is in Effect to [clang][AIX] Fix Overly Strict LTO Option Checking against `data-sections` when `mxcoff-roptr` is in Effect.Jun 14 2023, 12:49 PM

Revert to the DataSections logic. It is better not to change that in this patch.

LGTM with minor comment.

clang/lib/Driver/ToolChains/CommonArgs.cpp
732–735

Coding standards suggest that braces be used consistently for each part of an if/else chain.

This revision is now accepted and ready to land.Jul 3 2023, 8:40 PM

Fixing inconsistent braces. Thanks for the feedback @hubert.reinterpretcast !

qiongsiwu1 marked an inline comment as done.Jul 11 2023, 7:56 AM
This revision was landed with ongoing or failed builds.Jul 11 2023, 10:24 AM
This revision was automatically updated to reflect the committed changes.