[PATCH] D58930: Add XCOFF triple object format type for AIX

[Jason Liu via Phabricator via cfe-commits]

jasonliu marked 2 inline comments as done.
jasonliu added inline comments.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:2079
+          if (log)
+            log->Printf("sorry: unimplemented for XCOFF");
+          return false;
----------------
hubert.reinterpretcast wrote:
> davide wrote:
> > hubert.reinterpretcast wrote:
> > > JDevlieghere wrote:
> > > > jasonliu wrote:
> > > > > JDevlieghere wrote:
> > > > > > jasonliu wrote:
> > > > > > > apaprocki wrote:
> > > > > > > > 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.
> > > > > > > Sure. Will address in next revision.
> > > > > > Just bundle this with the WASM case, the error message is correct for both.
> > > > > 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.  
> > > > 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.
> > > > 
> > > 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.
> > 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 :)
> Well, I don't know about "soon"...
> Using the WASM message for XCOFF is not actually wrong; so, I can be okay with it.
Okay. Shared message it is. 


================
Comment at: llvm/lib/MC/MCContext.cpp:165
+    case MCObjectFileInfo::IsXCOFF:
+      // TODO: Need to implement class MCSymbolXCOFF.
+      break;
----------------
sfertile wrote:
> jasonliu wrote:
> > JDevlieghere wrote:
> > > See previous comment.
> > 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. 
> 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?
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. 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58930/new/

https://reviews.llvm.org/D58930