[PATCH] D137787: [CodeGen] Relax assertion on generating destructor call

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D137787#3921673 <https://reviews.llvm.org/D137787#3921673>, @Hahnfeld wrote:

> I've trimmed the failing code down to
>
>   #include <string>
>   #include <string_view>
>   #include <vector>
>   
>   template <typename T>
>   struct SO {
>     void a() {
>       struct SI {
>         std::vector<int> v;
>       };
>       SI s;
>       SI m(std::move(s));
>     }
>   
>     void g() {
>       std::vector<std::string_view> v{"a"};
>     }
>   };
>
> in a header / module and
>
>   SO<int> s;
>   s.a();
>   s.g();
>
> in the calling code. Sadly this works fine in standalone Clang...
>
> All of the above code seems to be important, starting from the outer `template`, having two functions, moving a `std::vector` from a default generated move constructor and then constructing a `std::vector<std::string_view>` with at least one element. If this rings a bell for anybody or anybody has an idea where to go from here, please let me know. I'm out of depth how to produce the exact failing conditions in a test. I would argue that relaxing the `assert` is fine regardless because it still tests that the `DtorDecl` belongs to this type, but I can't articulate why an exact pointer comparison fails in very rare circumstances...

So the code look like:

  // foo.h
  #include <string>
  #include <string_view>
  #include <vector>
  
  template <typename T>
  struct SO {
    void a() {
      struct SI {
        std::vector<int> v;
      };
      SI s;
      SI m(std::move(s));
    }
  
    void g() {
      std::vector<std::string_view> v{"a"};
    }
  };
  
  // foo.cpp
  import "foo.h";
  void func() {
      SO<int> s;
      s.a();
      s.g();
  }

Is it the reproducer? If it is, my suggestion would be:

- Preprocess `foo.h` into `foo.ii` then replace `import "foo.h";`
- Then reduce `foo.ii` step by step. There should be many things that can be removed.
- Then we found there is no things we can reduce, then it should be the test.

I know it sounds too hard at first. But I tried and it works although it would take one whole day. Here is an example for the reduced test case: https://github.com/llvm/llvm-project/commit/1aaba40dcbe8fdc93d825d1f4e22edaa3e9aa5b1, which is reduced from 2 system headers which contains 30,000+ lines of code.

> I would argue that relaxing the `assert` is fine regardless

Yes, I understood. But we hope it can land with a test case really. Otherwise, the same use case may be broken some time again without being noticed by any one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137787