[PATCH] D77962: PR38490: Support reading values out of __uuidof(X) expressions during constant evaluation.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith marked 2 inline comments as done.
rsmith added a comment.

In D77962#1978668 <https://reviews.llvm.org/D77962#1978668>, @rnk wrote:

> This code is very string-y. Should clang be parsing uuids into u32-u16-u16-u8[8] earlier, so that we can get the semantic form directly from the UuidAttr? Based on the test cases you had to pass, is there any reason that would be difficult?


I think that's a good idea, and should be reasonably straightforward. (The CodeGen side of this, which also parses the UUID, is also stringy, as is the mangling side, and probably none of these three should be.)



================
Comment at: clang/lib/AST/ExprConstant.cpp:3949
+      // Special-case reading from __uuidof expressions so we don't have to
+      // construct an APValue for the whole uuid.
+      if (LVal.Designator.isOnePastTheEnd()) {
----------------
aeubanks wrote:
> For my knowledge, why try to avoid constructing an APValue for the whole uuid? Is special casing this necessary?
I don't think it's incredibly important. It matters a little bit for a case like:

```
constexpr MyGuid f(const GUID &g) {
  return {g.Data1, g.Data2, g.Data3, g.Data4[0], g.Data4[1], ..., g.Data4[7]};
}
MyGuid g = f(__uuidof(IUnknown));
```

... where if we always created a complete `APValue`, we'd produce the complete `APValue` 11 times. It also didn't seem to be any harder than always forming the complete `APValue` for the GUID.

If we create a new kind of `Decl` to represent a UUID and store the `APValue` there, we can remove nearly all the `CXXUuidofExpr` special casing in constant evaluation and template arguments. That might actually be pretty straightforward, and I want to do something very similar for template parameter objects for C++20 class type non-type template parameters. But we don't need to do that in order to fix this bug, so I'm mildly inclined to go with the special case here and then remove this code again by a refactoring.


================
Comment at: clang/test/CodeGenCXX/microsoft-uuidof.cpp:37
 // Make sure we can properly generate code when the UUID has curly braces on it.
-GUID thing = __uuidof(Curly);
+GUID thing = (side_effect(), __uuidof(Curly));
 // CHECK-DEFINE-GUID: @thing = global %struct._GUID zeroinitializer, align 4
----------------
rnk wrote:
> Do we need to add coverage of the constexpr-evaluated initializer codepath? Is it just a matter of updating the CHECKs, or do we reject this code now?
> 
> If we reject, will that create a migration problem for users?
We still accept the old form of this code, but would constant initialize it instead. Code generation for the constant initialization case is tested by `GUID const_init` below. Support for `constexpr` GUID variables initialized by `__uuidof` is covered in `test/SemaCXX/ms-uuid.cpp`.

I needed to change this code because we no longer generated a dynamic initializer for this variable (because we can now constant-evaluate it instead), and both this `CHECK` and the one below for the "static [sic] initializer for thing" would fail.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77962