This is an archive of the discontinued LLVM Phabricator instance.

[AIX][XCOFF] print unsupported message for llvm-ar big archive write operation
ClosedPublic

Authored by DiggerLin on Mar 30 2022, 10:07 AM.

Details

Summary

when run "llvm-ar cr" on AIX OS , it created a gnu archive, it is not desirable in aix OS.
instead of creating a gnu archive, the patch will print a unsupport message for llvm-ar big archive write operation in AIX OS.

after implement the big archive operation, I will revert the XFAIL: AIX " and "--format=gnu" test cases.

Diff Detail

Event Timeline

DiggerLin created this revision.Mar 30 2022, 10:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2022, 10:07 AM
Herald added a subscriber: rupprecht. · View Herald Transcript
DiggerLin requested review of this revision.Mar 30 2022, 10:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2022, 10:07 AM
DiggerLin edited the summary of this revision. (Show Details)Mar 30 2022, 11:28 AM
jhenderson requested changes to this revision.Mar 30 2022, 11:55 PM
jhenderson added inline comments.
llvm/test/tools/llvm-ar/default-xcoff.test
2

I'm confused why this is system-aix but the XFAILed tests are just aix. I supsect they all want to be system-aix?

3

Let's use the term "Big" with upper-case 'B' to distinguish it from just a large archive.

12

Nit: too many blank lines at EOF.

llvm/test/tools/llvm-link/archivell.ll
1 ↗(On Diff #419224)

What's the motivation for using --format=gnu in this and other test cases rather than the XFAIL option?

llvm/tools/llvm-ar/llvm-ar.cpp
948

llvm_unreachable is for use only when the program should never be able to reach this code. Clearly, here it can reach it (or you wouldn't need all those XFAILs in the tests). Instead, use fail like in the case BSD: case a few lines below.

Also, error messages should start with lower-case letter, as per the LLVM style guide.

1280

Undo this change. It's not relevant to the patch.

This revision now requires changes to proceed.Mar 30 2022, 11:55 PM
DiggerLin marked 5 inline comments as done.Mar 31 2022, 7:28 AM
DiggerLin added inline comments.
llvm/test/tools/llvm-link/archivell.ll
1 ↗(On Diff #419224)

Those test case do not test functionality of llvm-ar (archive), it test the llvm-link, llvm-readobj, or llvm-nm , I just want the test case go through in the AIX. the format is do not matter.

I will revert those test cases when I implement the writing operation of big archive.

DiggerLin updated this revision to Diff 419452.Mar 31 2022, 7:29 AM
DiggerLin marked an inline comment as done.

address James comment

This revision is now accepted and ready to land.Mar 31 2022, 10:34 PM
jsji accepted this revision.Apr 1 2022, 6:41 AM
jsji added a subscriber: jsji.

LGTM. Thanks @DiggerLin !

llvm/tools/llvm-ar/llvm-ar.cpp
948

nit: Can we update the message to be more verbose? eg: big archive writer operation on AIX not yet supported?

This revision was landed with ongoing or failed builds.Apr 1 2022, 8:55 AM
This revision was automatically updated to reflect the committed changes.
DiggerLin marked an inline comment as done.Apr 1 2022, 11:18 AM