[PATCH] D71144: [AIX] Use csect reference for function address constants

[David Tenty via Phabricator via llvm-commits]

daltenty added a comment.

Other than minor nit (and the follow on issue), this patch LGTM.

With regards to the testing:

In D71144#1791800 <https://reviews.llvm.org/D71144#1791800>, @hubert.reinterpretcast wrote:

> I am, however, concerned about the testing. We are adding code to create a `MCSectionXCOFF` with various properties in this patch, and we don't seem to have the corresponding testing here. For example, how would the `C_HIDDEN` manifest itself if not corrected by code inspection? It is also difficult to be confident that a test would actually exercise this instance of the code that sets the csect symbol properties (as opposed to another instance).


This is one reason for the possible refactor mentioned before that moves creating these properties of an MCSectionXCOFF to a common place. It is difficult to see how we would have adequate test coverage otherwise, as you say there are too many code paths involved that may end up imbuing  a particular MCSectionXCOFF with a different set of properties depending on how we got there. I think we should prioritize addressing this in a follow on patch.



================
Comment at: llvm/test/CodeGen/PowerPC/aix-reference-func-addr-const.ll:14
+
+;CHECK:          .csect .data[RW]
+;CHECK-NEXT:     .globl  foo_ptr
----------------
nit: You can probably fold most of these check directives together with the 64-bit case by using a CHECK32 for the 32-bits parts and specifying multiple check prefixes. We are currently doing this in other tests that vary only slightly based on 32/64-bit. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71144