[PATCH] D72973: [llvm-objdump] Use symbol index+symbol name + storage mapping class as label for -D

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:798
+  if (Ret)
+        assert(getNumberOfAuxEntries() >=1 &&
+           "No CSECT Auxiliary Entry is found.");
----------------
DiggerLin wrote:
> hubert.reinterpretcast wrote:
> > Sorry for the churn. Seeing it in this context makes me realize that this probably can't be an `assert` in the first place unless if know that the checking was already done. We will need a function without the `assert` to query whether or not we should expect to find a CSECT Auxiliary Entry (`hasCsectAuxExt` is not named well for that purpose). When we request a CSECT Auxiliary Entry, we can assert that we had a reason to expect that there is one and we can also pass an error back to the caller if there isn't one. Whether or not we can propagate said error without too much difficulty will depend on where the caller is.
> I think assert(getNumberOfAuxEntries() >=1 is OK ,here , because the function is hasCsectAuxEnt() , The function names means it should at least has one Csect Aux entry. 
> 
> we can return Expected<bool> for the function. 
> 
> I am prefer to change it in NFC patch.  because there are some place using the function too.(it is not in the scope the patch) . and the return type of function getXCOFFCsectAuxEnt32  maybe need to change to expect<const XCOFFCsectAuxEnt32 *>
> 
> Maybe we also need a new function in the NFC patch
> 
> bool XCOFFSymbolRef::withCsectAuxEnt() const {
>   XCOFF::StorageClass SC = getStorageClass();
>   return (SC == XCOFF::C_EXT || SC == XCOFF::C_WEAKEXT ||
>           SC == XCOFF::C_HIDEXT);
> } 
The way it is named, if there isn't a CSECT Auxiliary Entry, then the result of this function should just be `false`. That's not what we are looking for. This function is used (as you also noted) in other places. We can leave the function name and implementation unchanged and rename it in a separate patch. We can also change `getXCOFFCsectAuxEnt32` to be able to express an error condition in that patch. In this patch, please add a TODO for each of these changes where the affected function is defined.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:799
+  if (Ret)
+        assert(getNumberOfAuxEntries() >=1 &&
+           "No CSECT Auxiliary Entry is found.");
----------------
DiggerLin wrote:
> jhenderson wrote:
> > clang-format is still complaining here.
> thanks
As far as I know, the assert may fail due to the XCOFF file being invalid. Please remove the assert and add the TODO comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72973