In D65761#1814968, @ajpaverd wrote:

The CFGuard library shouldn't be needed for targets other than ARM, AArch64, and X86, and it's only being built for these targets. However, it looks like llvm-build is also including it in the LibraryDependencies.inc file for other targets, such as PowerPC, which I think is causing this error. I'm not sure how to indicate that the library is only needed for a specified subset of targets. @rnk or @hans any suggestions?

LGTM.

@stevewan, can you confirm if there are other tools/utilities that need similar treatment?

In D68115#1820462, @vitalybuka wrote:

So if I understand this the proposal is to have something like -fzero-union-padding which is off by default.
When it's OFF compiler will continue to do whatever it does now.
When it's ON it will set zeroes into padding with or without -ftrivial-auto-var-init.
Is this correct?

LGTM with just the fix to replace the "maybe"s. What we should do for the later TODO can be determined later.

LGTM.

The test needs some minor changes that can be fixed on the commit. LGTM otherwise.

Some minor nits that can be fixed on the commit, but otherwise LGTM.

In D72369#1812104, @jhenderson wrote:

I'd still really like to see a test that tests the other code path that tries to print DW_LNS_* (see the inline comment). The obvious thing to me to do would be to update the unit test "ParserPrintsStandardOpcodesWhenRequested", but it would require changes to the DwarfGenerator to create a custom prologue. If you don't want to do this, that's okay, as the code path is already untested.

In D72369#1809613, @jhenderson wrote:

I think a better solution is to print something more meaningful. For example, "DW_LNS_unknown_<value>" or something similar. You can see examples of that with the DW_AT_unknown_* printing in debug-abbrev.s. I don't think that will be significantly more complex and will provide more user-friendly output.

DWARFDebugLine.cpp: Format unknown line number standard opcodes

In D65761#1804302, @hubert.reinterpretcast wrote:

I have confirmed that the case I mentioned fails with rGd157a9b.

In D68115#1810891, @lebedev.ri wrote:

Does this have to be an unilateral change,
likely penalizing non--ftrivial-auto-var-init= cases,
i.e. [why] can't it be only done for when -ftrivial-auto-var-init= is enabled?

We left off near that conclusion (https://reviews.llvm.org/D68115#1686887); however, -ftrivial-auto-var-init= merely increases the chance that code expecting the zeroing to occur would break. A separate option to control zeroing for union padding would help in cases where the zeroing does not happen for reasons other than -ftrivial-auto-var-init.

@daltenty, following up on needing to set linkage for csects in the assembly (in a more general case than just for function descriptors, as you pointed out): We are missing the linkage on the assembly path for weak references.

@jasonliu, @daltenty; as discussed off-list today, it would be much cleaner if we dropped producing a label definition for the function descriptor. In particular, I have validated my suspicions that referencing the DS csect in a file where the function is defined weak is problematic if the linkage is associated with the label definition and not the csect. Thank you both for tracking this on your TODO lists.

LGTM.

Is building check-all immediately after configuration expected to be clean? I have a build configured to build LLVM with just clang and just with the PowerPC target, and it seems check-all (under such a configuration) does not trigger the building of the library added in this patch; which, in turn, leads to test failures.

In D71504#1793240, @sfertile wrote:

I think if the front-end AIX support was further along then a patch like this should be deferred until we fixed (and verified) the IR the front-end generates for sret arguments. With how clang/CodeGen is going through the SVR4 targets for AIX its safe to consider any Clang AIX support as extremely experimental right now, and the back-end implementation shouldn't be held up based on Clang support for the target.

@sfertile, I'm not seeing the change to the test to add -verify-machineinstrs. Perhaps you have a newer version to upload?

Just one minor issue with the patch itself, and otherwise, I don't see an issue with the patch in isolation.

Looks fine here as well. Not sure if other reviewers would have comments though.

In D71504#1787758, @ZarkoCA wrote:

I don't know what happens in the case where the front-end doesn't handle some cases correctly but the back-end does. Is there a danger in enabling this in the back-end knowing we don't get correct IR for complex types for AIX? Is there an error in the front-end for those before we get to this step?

There's no error from the front-end. In terms of continuous delivery, this leads to silently bad code that could lead to incorrect output or crashes at runtime.

@daltenty, please update the commit message to indicate the corrected scope. This LGTM insofar as it fits the current handling of function descriptors for which we have a definition in the current object.

In D71504#1785698, @ZarkoCA wrote:

So it doesn't look like we need to add any special handling for struct returns on AIX? I had a quick look and none of the other PPC targets seem to have anything.

The scope of the "drive-by" change is heading into redesign territory.

Committed as rG93faa237da8d. @sfertile, typo in the commit message: s/Differtential/Differential/;

The commit subject should be more direct in indicating what the patch does, not what problem it solves.

In D70724#1780372, @ZarkoCA wrote:

Nit: It may be better to rename the test aix-indirect-call.ll to be consistent.

I agree.

This patch might need splitting. At least the commit subject should encompass the scope more. Once the tests are added for weak external or internal linkage, the "drive-by" change will be sizable.

As Jason suggested, the code duplication can be reduced:

const auto ProduceDescriptorAndReplaceCallee =
    [&](StringRef FuncName, bool EnsureContainingCsectIsSet,
        const auto &GetStorageClass) {
      auto &Context = DAG.getMachineFunction().getMMI().getContext();

Some minor comments; otherwise, I think this is fine, but I don't know if others believe there are still comments to be addressed.