This is an archive of the discontinued LLVM Phabricator instance.

Add XCOFF triple object format type for AIX
ClosedPublic

Authored by jasonliu on Mar 4 2019, 3:22 PM.

Details

Summary

This patch adds an XCOFF triple object format type into LLVM. This XCOFF triple object file type will be used later by object file and assembly generation for the AIX platform.

Diff Detail

Repository
rL LLVM

Event Timeline

jasonliu created this revision.Mar 4 2019, 3:22 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptMar 4 2019, 3:22 PM
llvm/lib/Support/Triple.cpp
537 ↗(On Diff #189211)

If the conclusion is that checking XCOFF before COFF is fine, then we should remove the FIXME and just leave a normal comment.

apaprocki added inline comments.Mar 5 2019, 5:54 AM
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
2079 ↗(On Diff #189211)

No need to be sorry: :) This should probably just say error: XCOFF is unimplemented to be more direct in case anything is expecting "error:" in the output.

llvm/lib/Support/Triple.cpp
537 ↗(On Diff #189211)

Agreed, existing code seems fine as long as there is a comment explaining that xcoff must come before coff in case it isn't obvious at a quick glance.

jasonliu marked 2 inline comments as done.Mar 5 2019, 11:28 AM
jasonliu added inline comments.
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
2079 ↗(On Diff #189211)

Sure. Will address in next revision.

llvm/lib/Support/Triple.cpp
537 ↗(On Diff #189211)

Sounds good. I will remove FIXME and leave a normal comment to indicate the order dependency.

jasonliu updated this revision to Diff 189387.Mar 5 2019, 12:41 PM

Address some review comments

jasonliu marked an inline comment as done.Mar 5 2019, 12:43 PM
JDevlieghere requested changes to this revision.Mar 5 2019, 12:45 PM
JDevlieghere added a subscriber: JDevlieghere.
JDevlieghere added inline comments.
clang/lib/CodeGen/BackendUtil.cpp
1470 ↗(On Diff #189211)

This is confusing, you say fall through but you break? I would prefer a llvm_unreachable("XCOFF not yet implemented"); here and elsewhere in this patch.

1486 ↗(On Diff #189211)

See previous comment.

clang/lib/CodeGen/CodeGenModule.cpp
4410 ↗(On Diff #189211)

See previous comment.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
2079 ↗(On Diff #189211)

Just bundle this with the WASM case, the error message is correct for both.

llvm/lib/MC/MCContext.cpp
165 ↗(On Diff #189211)

See previous comment.

This revision now requires changes to proceed.Mar 5 2019, 12:45 PM
jasonliu marked an inline comment as done.Mar 5 2019, 12:50 PM
jasonliu added inline comments.
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
2079 ↗(On Diff #189211)

I think they are different.
The error message for WASM seems to suggest that it will never ever get supported on WASM.
But it is not the case for XCOFF, we want to indicate that it is not implemented yet.

jasonliu marked 3 inline comments as done.Mar 5 2019, 12:57 PM
jasonliu added inline comments.
clang/lib/CodeGen/BackendUtil.cpp
1470 ↗(On Diff #189211)

Thanks for the comment. Will address in next revision.

1486 ↗(On Diff #189211)

Will address in next revision.

clang/lib/CodeGen/CodeGenModule.cpp
4410 ↗(On Diff #189211)

Will address in next revision.

jasonliu marked an inline comment as done.Mar 5 2019, 3:17 PM
jasonliu added inline comments.
llvm/lib/MC/MCContext.cpp
165 ↗(On Diff #189211)

It is certain that we will need MCSymbolXCOFF. But before we run into cases where we actually need a MCSymbolXCOFF, we could use the generic MCSymbol first for XCOFF platform. So I don't want to put a llvm_unreachable here.

JDevlieghere added inline comments.Mar 5 2019, 3:31 PM
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
2079 ↗(On Diff #189211)

I don't think the error message suggests that at all, and it's definitely not true. At this point neither XCOFF nor WASM is supported, and that's exactly what the log message says.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
2079 ↗(On Diff #189211)

I agree that the error message for WASM does not indicate that the lack of support is inherent or intended to be permanent; however, it is not indicative either of an intent to implement the support. I am not sure what the intent is for WASM, but I do know that the intent for XCOFF is to eventually implement the support. I do not see how using an ambiguous message in this commit (when we know what the intent is) is superior to the alternative of having an unambiguous message.

jasonliu updated this revision to Diff 189514.Mar 6 2019, 8:27 AM

Address comments in last revision.

davide requested changes to this revision.Mar 6 2019, 8:51 AM
davide added a subscriber: davide.
davide added inline comments.
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
2079 ↗(On Diff #189211)

I think we should keep this consistent with the other target so my vote is for grouping XCOFF with WASM. After all, if it's going to be implemented soon, the message will go away :)

This revision now requires changes to proceed.Mar 6 2019, 8:51 AM
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
2079 ↗(On Diff #189211)

Well, I don't know about "soon"...
Using the WASM message for XCOFF is not actually wrong; so, I can be okay with it.

sfertile added inline comments.Mar 6 2019, 10:02 AM
llvm/lib/MC/MCContext.cpp
165 ↗(On Diff #189211)

Would it make sense to add an llvm_unreachable now, and the first patch that actually uses an MCSymbol stubs out the class and removes the unreachable?

jasonliu marked 2 inline comments as done.Mar 6 2019, 11:03 AM
jasonliu added inline comments.
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
2079 ↗(On Diff #189211)

Okay. Shared message it is.

llvm/lib/MC/MCContext.cpp
165 ↗(On Diff #189211)

The first patch that uses MCSymbol do not necessarily need to stub out MCSymbolXCOFF, as MCSymbol seems to be generic and usable until we are doing some XCOFF specific things that needs to be represented by MCSymbolXCOFF. If the intention is MCSymbol should never get used, and different object file should have their own MCSymbolXXX class from the start, then I could add in the llvm_unreachable here, and I would also propose to replace the "return new (Name, *this) MCSymbol(MCSymbol::SymbolKindUnset, Name, IsTemporary);" with an llvm_unreachable as well.

jasonliu updated this revision to Diff 189713.Mar 7 2019, 7:10 AM

Share the same "unsupported error" message with WASM.

sfertile accepted this revision.Mar 8 2019, 7:46 AM

LGTM.

llvm/lib/MC/MCContext.cpp
165 ↗(On Diff #189211)

Ok I think falling through to create the generic MCSymbol in this patch is reasonable.

@jasonliu, you have had a number of patches committed into the project already (D22698, D22702, D34649). Please go ahead with requesting commit access, and commit this patch with the additional fix when you are ready.

llvm/lib/MC/MCObjectFileInfo.cpp
806 ↗(On Diff #189713)

This line is over 80 characters.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 12 2019, 3:00 PM
This revision was automatically updated to reflect the committed changes.