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

[Alexander Richardson via Phabricator via cfe-commits]

arichardson added a comment.

In D78491#2040424 <https://reviews.llvm.org/D78491#2040424>, @dylanmckay wrote:

> 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?


I would love to do that, but it would require many hundreds of changes. This is why I decided to go for a per-directory opt-in approach. 
The next step would be to change it to an opt-out and finally remove it.

I can try inverting the logic to explicitly request implicit address space zero and see how many CMakeLists.txt need changing.
The benefit of that approach would be that new directories will automatically require explicit address spaces.

One reason the default AS macro might be useful is for out-of-tree consumers of the LLVM libraries that always use address space zero and changing it would require large update costs to move to LLVM 11+.
I'm not sure that's a reason though since the APIs are not considered stable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78491