[CodeGen] Disable default copy ctor and copy assignment operator for class Array

[yubing]

Summary

class Array manages resources such as dynamically allocated memory, it's generally a good practice to either implement a custom copy constructor or disable the default one.

[pengfei]

LGTM.

[dblaikie]

Any particular motivation for this series of patches? (like this non-owning Array type looks totally fine to copy, like ArrayRef)

[aditya_nandakumar]

We actually have some out of tree code that requires us to dynamically grow LiveRegMatrix (for compile time optimizations for our backend) and we rely on these copy constructors/assignment operators to resize the Matrix. Like David mentioned above, what's the motivation here? It should be perfectly fine to update pointers and size that's not being owned by this class?

[aditya_nandakumar]

Can you please revert this commit unless there's a strong motivating reason for this commit?

[yubing]

@dblaikie @aditya_nandakumar i just revert the commit

[dblaikie]

In D152231#4426778, @yubing wrote:

@dblaikie @aditya_nandakumar i just revert the commit

Could you, perhaps, revisit the other similar patches in this series - maybe a discourse post summarizing/linking to them all & explaining the general motivation for them/which ones we should keep/revert/etc and why?

[pengfei]

I think the intention here is to harden the code to avoid potential risks, e.g., UAF, caused by shallow copy.
I don't see a problem to have a strict rule for this. According to LLVM policy, downstream only usage is not a strong reason for upstream code. And I think it's also good to define an explicit copy constructor if you are intended to do the copy.

[barannikov88]

In D152231#4427054, @pengfei wrote:

I think the intention here is to harden the code to avoid potential risks, e.g., UAF, caused by shallow copy.
I don't see a problem to have a strict rule for this. According to LLVM policy, downstream only usage is not a strong reason for upstream code. And I think it's also good to define an explicit copy constructor if you are intended to do the copy.

How about making them ' = default' instead?

[dblaikie]

In D152231#4427054, @pengfei wrote:

I think the intention here is to harden the code to avoid potential risks, e.g., UAF, caused by shallow copy.
I don't see a problem to have a strict rule for this. According to LLVM policy, downstream only usage is not a strong reason for upstream code. And I think it's also good to define an explicit copy constructor if you are intended to do the copy.

But there was no problem with this code and its copy ctor/assignment operator to begin with? Perhaps just renaming this or reusing the exsiting llvm::MutableArrayRef here?
& I'm still not sure about the rest of the related changes that were made to other types in this series of patches - I'd like a broader design discussion about the set of changes, probably on discourse.

[skan]

In D152231#4427275, @barannikov88 wrote:

In D152231#4427054, @pengfei wrote:

I think the intention here is to harden the code to avoid potential risks, e.g., UAF, caused by shallow copy.
I don't see a problem to have a strict rule for this. According to LLVM policy, downstream only usage is not a strong reason for upstream code. And I think it's also good to define an explicit copy constructor if you are intended to do the copy.

How about making them ' = default' instead?

I have the same suggestion

[dblaikie]

In D152231#4428908, @dblaikie wrote:

In D152231#4427054, @pengfei wrote:

I think the intention here is to harden the code to avoid potential risks, e.g., UAF, caused by shallow copy.
I don't see a problem to have a strict rule for this. According to LLVM policy, downstream only usage is not a strong reason for upstream code. And I think it's also good to define an explicit copy constructor if you are intended to do the copy.

But there was no problem with this code and its copy ctor/assignment operator to begin with? Perhaps just renaming this or reusing the exsiting llvm::MutableArrayRef here?
& I'm still not sure about the rest of the related changes that were made to other types in this series of patches - I'd like a broader design discussion about the set of changes, probably on discourse.

Ping on all this.

llvm/include/llvm/CodeGen/LiveIntervalUnion.h

#endif
 
   // Array of LiveIntervalUnions.
   class Array {
     unsigned Size = 0;
     LiveIntervalUnion *LIUs = nullptr;
 
   public:
     Array() = default;
+    Array &operator=(const Array &other) = delete;
+    Array(const Array &other) = delete;
     ~Array() { clear(); }
 
     // Initialize the array to have Size entries.
     // Reuse an existing allocation if the size matches.
     void init(LiveIntervalUnion::Allocator&, unsigned Size);
 
     unsigned size() const { return Size; }