[PATCH] D78491: Avoid relying on address space zero default parameter in llvm/IR

[Dylan McKay via Phabricator via cfe-commits]

dylanmckay added a comment.
Herald added a project: LLVM.

I really like the idea behind this patch, but I suspect there is a better way to go about it.

The only drawbacks to upstreaming this patch as-is is the idea that because it is implemented as a compiler define directive, it effectively splinters the the core LLVM C++ API into two different versions. Of course, this is kind of the point, but my only worry is that the `LLVM_DEFAULT_AS_PARAM` macro adds an additional layer of complexity to all users of the API that will be encountered and must be considered.

I think, if we have already identified all the call sites that break under the `LLVM_DEFAULT_AS_PARAM` patch, then we should instead make two final patches that fix the root rather than add the `LLVM_DEFAULT_AS_PARAM` for retroactive discovery. 1) replace all calls to `getUnqual` with the appropriate logic, which it looks like you've already done, and 2) remove the default value of `0` from all address space parameters so the callers *must* explicitly pass the address space in. I think that 2) would be hard to swing in the past whilst AVR was an experimental target, but now that AVR is an officially supported LLVM target, it is invariably true that all calls to `getOrCreateInternalVariable` and friends without explicit address spaces will break an officially supported in-tree target. It no longer makes sense to default to zero, so let's fix the root cause.

This would the problem at the root, stopping all new LLVM code from implicitly defaulting to address space `0`. This still leaves open the problem of developers explicitly calling `getUnqual` when constructing pointers, but this problem is left open under this patch currently as regardless of the default parameter or not, calls to `getUnqual` are still allowed.

**tl;dr** I suggest removing the `LLVM_DEFAULT_AS_PARAM` directive from this patch, instead permanently removing the default value of `= 0`. This will achieve the same end result, but it will fix the broken API rather than just making it easier for downstream users like CHERI to discover.

What are your thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78491