pgiffuni commented on PR #512: URL: https://github.com/apache/openoffice/pull/512#issuecomment-6374301492
Here is a quick review (from ChatGPT): I do not see a reason to reject the overall approach: the ELFv2 return-value correction is necessary, and the reported FreeBSD tests are encouraging. 1. High priority: homogeneous floating-point aggregate detection is too permissive File: main/bridges/source/cpp_uno/gcc3_freebsd_powerpc64/uno2cpp.cxx Function: collect_fpr_members() The new helper recursively collects structure members and accepts a structure when every leaf member is either float or double and all leaves have the same type. It limits the total to eight members. That is a useful starting point, but it does not verify the actual member layout or the absence of padding. The PowerPC64 ELFv2 ABI defines homogeneous aggregates in terms of the aggregate's layout as well as its element types; its specification explicitly excludes certain aggregates containing padding words. [Kib](https://kib.kiev.ua/x86docs/POWER/ABI/leabi-20201201.pdf?utm_source=chatgpt.com) The concern is that the bridge could classify a type as an HFA based on its members even when its memory layout does not match the sequence of floating-point values the bridge subsequently copies into or out of FPRs. Recommended change: - Verify member offsets and element sizes, not just member types. - Ensure the complete aggregate layout is compatible with the flattened sequence. - Reject unsupported layouts and use the ordinary aggregate-return classification instead. - Include inherited structures and nested structures in the tests. I would treat this as a classification correctness issue to resolve, rather than claiming that a specific existing UNO type is already broken. 2. High priority: the tests should exercise both directions of every new return convention Files: uno2cpp.cxx, cpp2uno.cxx, call.s The patch changes both directions of the bridge: - C++ calling into UNO, where the bridge must reconstruct returned values. - UNO calling into C++, where the bridge must capture the native return registers. It also introduces a special return code, TYPECLASS_FPRS = 100, to tell privateSnippetExecutor to restore floating-point return registers. That convention crosses the C++/assembly boundary, so a mismatch could be difficult to diagnose. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
