[PATCH] D92398: [AIX][XCOFF] emit traceback table for function in aix

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:139-141
+  assert(I == ParmsNum &&
+         "The total parameters number of fixed-point or floating-point "
+         "parameters not equal to the number in the parameter type!");
----------------
DiggerLin wrote:
> hubert.reinterpretcast wrote:
> > The message text here is wrong. The correct `ParmsNum` here needs to include the number of vector parameters, which the XL compilers consider as neither "fixed-point" nor floating-point.
> > 
> > @DiggerLin, please confirm that the caller of this function correctly includes the vector parameter count.
> > 
> > Also, same comment as above re: `assert`.
> yes, the ParmsNum in function  only includes the fixed  and floating parameter count currently. 
> so in the function , when decoding we  do not have  ++I in the 
> "case TracebackTable::ParmTypeIsVectorBits" , we  grantee all the vector parameter meter will be decoded by the  Value != 0u . actually if we only put fixed parameter count as parameter of the function, we can also decode the ParmsType correctly by Value != 0u.
> 
> that is why the  message text is "The total parameters number of fixed-point or floating-point " not have vector ,  
> 
> if you not agreed with only pass  fixed  and floating parameter  count, I can create a NFC patch later.
> yes, the ParmsNum in function  only includes the fixed  and floating parameter count currently. 
This is an interface surprise that should have copious comments if it were to be left around.

> so in the function , when decoding we  do not have  ++I in the 
> "case TracebackTable::ParmTypeIsVectorBits" , we  grantee all the vector parameter meter will be decoded by the  Value != 0u .
If this is true, then it's worth having a comment where the `++I` is intentionally omitted.

> actually if we only put fixed parameter count as parameter of the function, we can also decode the ParmsType correctly by Value != 0u.
There are 8 GPRs and 12 VRs for function arguments. So, just with registers, there can be more than the 16 parameters that can be encoded in the 32 bits. The assert can be hit and it's not always an error. Trying to say that there is an infinite number of `0` bits to the right may be too clever because it makes it difficult to draw the line for other unambiguous situations involving having only vector parameters left to account for. An alternative solution where the number of encoding bits left is tracked and unencoded parameters are represented by an ellipsis would be something to consider.

> 
> that is why the  message text is "The total parameters number of fixed-point or floating-point " not have vector ,  
> 
> if you not agreed with only pass  fixed  and floating parameter  count, I can create a NFC patch later.
Thanks for your enlightening response. I think we have more to fix here.


================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:136
+    default:
+      assert(false && "Unrecognized bits in ParmsType.");
+    }
----------------
DiggerLin wrote:
> hubert.reinterpretcast wrote:
> > Is `assert` right for all expected callers of this function? This seems to be a reusable implementation where the error handling policy should be left to the caller.
> the code should not came to default.  added assert here we don't get surprised.
Okay; this is not just "should". It's actually impossible unless if the constants are incorrectly defined. Got it; thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92398