tbaeder marked 14 inline comments as done.
tbaeder added inline comments.
================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:749-751
+ const Descriptor::MetadataSize MDSize{sizeof(InlineDescriptor)};
+ Descriptor *D =
+ P.createDescriptor(Src, Ty, MDSize, IsConst, Src.is<const Expr *>());
----------------
erichkeane wrote:
> aaron.ballman wrote:
> > Double checking that `Src.is<const Expr *>()` is correct here. That's being
> > passed as the `IsTemporary` argument. I presume the logic is that if the
> > decl type is an expression then it has to be a temporary, but that's not
> > entirely true. Consider C where a compound literal is *not* a temporary but
> > actually creates an lvalue: `(int){12}`. Will that be an issue?
> See suggested comment below in InterpBlock.
That interpretation is correct and I don't know what to do about compound
literals yet.
================
Comment at: clang/lib/AST/Interp/Descriptor.cpp:207
: Source(D), ElemSize(primSize(Type)), Size(ElemSize * NumElems),
- AllocSize(align(Size) + sizeof(InitMap *)), IsConst(IsConst),
- IsMutable(IsMutable), IsTemporary(IsTemporary), IsArray(true),
- CtorFn(getCtorArrayPrim(Type)), DtorFn(getDtorArrayPrim(Type)),
- MoveFn(getMoveArrayPrim(Type)) {
+ MDSize(MD.NumBytes), AllocSize(align(Size) + sizeof(InitMap *) + MDSize),
+ IsConst(IsConst), IsMutable(IsMutable), IsTemporary(IsTemporary),
----------------
erichkeane wrote:
> Theres enough math on stuff like AllocSize/etc that we should probably:
>
> 1- do SOMETHING to make sure we're not overflowing
> 2- Have assertions to make sure we got the sizes looking reasonable.
>
> Additionally, is there any ability to refactor out the common subset of
> initializers into a constructor and delegate to them instead of all these
> really fragile looking changes?
I can try to factor it out into a common constructor. As for overflows, there
is code in `Program.cpp` to handle that (kinda, it doesn't take into account
the initmap and matadata size, as far as I can see).
================
Comment at: clang/lib/AST/Interp/Descriptor.h:74
+ };
+ static constexpr MetadataSize NoMetadata = MetadataSize{0};
+
----------------
shafik wrote:
> erichkeane wrote:
> > add a line:
> > `static constexpr MetadataSize InlineDescriptor =
> > MetadataSize{sizeof(InlineDescriptor)};`
> > and you can use this instead of depending on 'sizeof' all over the place.
> It feels weird to call this `NoMetadata` but we will pass this as an argument
> to a function with a parameter of `MetaDataSize`. So I am expecting a size
> but I am getting no meta data instead and it looks like a mistake.
>
> Maybe a better name would be `ZeroMetaDataSize`?
I kinda get what your point is, but I don't think this is confusing.
================
Comment at: clang/lib/AST/Interp/Descriptor.h:74
+ };
+ static constexpr MetadataSize NoMetadata = MetadataSize{0};
+
----------------
tbaeder wrote:
> shafik wrote:
> > erichkeane wrote:
> > > add a line:
> > > `static constexpr MetadataSize InlineDescriptor =
> > > MetadataSize{sizeof(InlineDescriptor)};`
> > > and you can use this instead of depending on 'sizeof' all over the place.
> > It feels weird to call this `NoMetadata` but we will pass this as an
> > argument to a function with a parameter of `MetaDataSize`. So I am
> > expecting a size but I am getting no meta data instead and it looks like a
> > mistake.
> >
> > Maybe a better name would be `ZeroMetaDataSize`?
> I kinda get what your point is, but I don't think this is confusing.
I'm a bit concerned about the naming here; `InlineDescriptor` is already a type
name, but calling it `MetadataSizeInlineDesc` is pretty long and callers
already have to prepend a `Descriptor::` to that. :/
================
Comment at: clang/lib/AST/Interp/Interp.h:950
return false;
if (auto *FD = Ptr.getField()) {
Ptr.deref<T>() = Value.truncate(FD->getBitWidthValue(S.getCtx()));
----------------
aaron.ballman wrote:
> Do we need to `initialize()` here as well?
I think so, but bitfields are currently completely untested.
================
Comment at: clang/lib/AST/Interp/InterpBlock.h:77-86
+ char *data() {
+ // Data might contain metadata as well.
+ size_t DataOffset = Desc->getMetadataSize();
+ return reinterpret_cast<char *>(reinterpret_cast<char *>(this) +
+ sizeof(Block) + DataOffset);
+ }
+
----------------
aaron.ballman wrote:
> Should we have `const`-correct overloads to provide read-only access?
IIRC I added that at least once in a different patch but I abandoned that and
haven't had a use for a const overload since.
================
Comment at: clang/lib/AST/Interp/InterpBlock.h:80-88
+ return reinterpret_cast<char *>(reinterpret_cast<char *>(this) +
+ sizeof(Block) + DataOffset);
+ }
+
+ /// Returns a pointer to the raw data, including metadata.
+ /// You are allowed to read Desc->getAllocSize() bytes from this address.
+ char *rawData() {
----------------
aaron.ballman wrote:
> Why two levels of `reinterpret_cast` to the same type?
No reason. The outer cast isn't needed.
================
Comment at: clang/lib/AST/Interp/InterpBlock.h:97
void invokeCtor() {
- std::memset(data(), 0, getSize());
+ std::memset(rawData(), 0, Desc->getAllocSize());
if (Desc->CtorFn)
----------------
aaron.ballman wrote:
> Why do we want to overwrite the metadata here?
This is only called after creating an new block, so nothing is being
overwritten, the metadata hasn't been filled-in yet.
================
Comment at: clang/lib/AST/Interp/Pointer.h:50
+///
+/// Pointee Offset
+/// │ │
----------------
shafik wrote:
> I feel like the comment block in `InterpBlock.h` is more informative.
The content might overlap, but they don't describe what `Offset` and `Base`
mean.
================
Comment at: clang/lib/AST/Interp/Pointer.h:347
assert(Offset != 0 && "Not a nested pointer");
- return reinterpret_cast<InlineDescriptor *>(Pointee->data() + Offset) - 1;
+ return reinterpret_cast<InlineDescriptor *>(Pointee->rawData() + Offset) -
+ 1;
----------------
shafik wrote:
> Why the `-1`
The inline descriptor is located //before// the `Base` (which is what's passed
as `Offset` here). See the comment block added in `Pointer.h`.
================
Comment at: clang/lib/AST/Interp/Program.cpp:334
Descriptor *ElemDesc =
- createDescriptor(D, ElemTy.getTypePtr(), IsConst, IsTemporary);
+ createDescriptor(D, ElemTy.getTypePtr(), Descriptor::NoMetadata,
+ IsConst, IsTemporary);
----------------
aaron.ballman wrote:
> Why does this one get no metadata while the rest of the descriptors created
> do?
This is the descriptor for the array elements. The metadata passed to this
function is only being used for the outermost descriptor, i.e. we use it a few
lines below for the actual array but we don't use it for the array elements.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135750/new/
https://reviews.llvm.org/D135750
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
