[llvm] [clang] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

@@ -79,6 +79,73 @@ using namespace llvm;
 
 namespace {
 
+// Created on demand if the coro-early pass has work to do.
+class Lowerer : public coro::LowererBase {
+  IRBuilder<> Builder;
+  void lowerAwaitSuspend(CoroAwaitSuspendInst *CB);
+
+public:
+  Lowerer(Module ) : LowererBase(M), Builder(Context) {}
+
+  void lowerAwaitSuspends(Function );
+};
+
+void Lowerer::lowerAwaitSuspend(CoroAwaitSuspendInst *CB) {
+  auto Helper = CB->getHelperFunction();
+  auto Awaiter = CB->getAwaiter();
+  auto FramePtr = CB->getFrame();
+
+  Builder.SetInsertPoint(CB);
+
+  CallBase *NewCall = nullptr;
+  if (auto Invoke = dyn_cast(CB)) {
+auto HelperInvoke =
+Builder.CreateInvoke(Helper, Invoke->getNormalDest(),
+ Invoke->getUnwindDest(), {Awaiter, FramePtr});
+
+HelperInvoke->setCallingConv(Invoke->getCallingConv());
+std::copy(Invoke->bundle_op_info_begin(), Invoke->bundle_op_info_end(),
+  HelperInvoke->bundle_op_info_begin());
+AttributeList NewAttributes =
+Invoke->getAttributes().removeParamAttributes(Context, 2);
+HelperInvoke->setAttributes(NewAttributes);
+HelperInvoke->setDebugLoc(Invoke->getDebugLoc());
+NewCall = HelperInvoke;
+  } else if (auto Call = dyn_cast(CB)) {
+auto HelperCall = Builder.CreateCall(Helper, {Awaiter, FramePtr});
+
+AttributeList NewAttributes =
+Call->getAttributes().removeParamAttributes(Context, 2);
+HelperCall->setAttributes(NewAttributes);
+HelperCall->setDebugLoc(Call->getDebugLoc());
+NewCall = HelperCall;
+  }
+
+  CB->replaceAllUsesWith(NewCall);
+  CB->eraseFromParent();
+
+  InlineFunctionInfo FnInfo;
+  auto InlineRes = InlineFunction(*NewCall, FnInfo);
+  assert(InlineRes.isSuccess() && "Expected inlining to succeed");
+  (void)InlineRes;
+}
+
+void Lowerer::lowerAwaitSuspends(Function ) {

ChuanqiXu9 wrote:

We can collect CoroAwaitSuspendInst during the construction of Shape. So that 
we can save one iterations of the function.

https://github.com/llvm/llvm-project/pull/79712
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

@@ -79,6 +79,73 @@ using namespace llvm;
 
 namespace {
 
+// Created on demand if the coro-early pass has work to do.
+class Lowerer : public coro::LowererBase {

ChuanqiXu9 wrote:

Why do we need to inherit `coro::LowererBase`? Can't we make it directly in a 
function?

https://github.com/llvm/llvm-project/pull/79712
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

@@ -338,6 +386,85 @@ static QualType getCoroutineSuspendExprReturnType(const 
ASTContext ,
 }
 #endif
 
+llvm::Function *CodeGenFunction::generateAwaitSuspendHelper(

ChuanqiXu9 wrote:

For such function definitions, generally we need a comment about its reasoning 
and behavior. This is not for me but other developers.

https://github.com/llvm/llvm-project/pull/79712
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

@@ -79,6 +79,73 @@ using namespace llvm;
 
 namespace {
 
+// Created on demand if the coro-early pass has work to do.
+class Lowerer : public coro::LowererBase {
+  IRBuilder<> Builder;
+  void lowerAwaitSuspend(CoroAwaitSuspendInst *CB);
+
+public:
+  Lowerer(Module ) : LowererBase(M), Builder(Context) {}
+
+  void lowerAwaitSuspends(Function );
+};
+
+void Lowerer::lowerAwaitSuspend(CoroAwaitSuspendInst *CB) {
+  auto Helper = CB->getHelperFunction();
+  auto Awaiter = CB->getAwaiter();
+  auto FramePtr = CB->getFrame();
+
+  Builder.SetInsertPoint(CB);
+
+  CallBase *NewCall = nullptr;
+  if (auto Invoke = dyn_cast(CB)) {
+auto HelperInvoke =
+Builder.CreateInvoke(Helper, Invoke->getNormalDest(),
+ Invoke->getUnwindDest(), {Awaiter, FramePtr});
+
+HelperInvoke->setCallingConv(Invoke->getCallingConv());
+std::copy(Invoke->bundle_op_info_begin(), Invoke->bundle_op_info_end(),
+  HelperInvoke->bundle_op_info_begin());
+AttributeList NewAttributes =
+Invoke->getAttributes().removeParamAttributes(Context, 2);
+HelperInvoke->setAttributes(NewAttributes);
+HelperInvoke->setDebugLoc(Invoke->getDebugLoc());
+NewCall = HelperInvoke;
+  } else if (auto Call = dyn_cast(CB)) {
+auto HelperCall = Builder.CreateCall(Helper, {Awaiter, FramePtr});
+
+AttributeList NewAttributes =
+Call->getAttributes().removeParamAttributes(Context, 2);
+HelperCall->setAttributes(NewAttributes);
+HelperCall->setDebugLoc(Call->getDebugLoc());
+NewCall = HelperCall;
+  }
+
+  CB->replaceAllUsesWith(NewCall);
+  CB->eraseFromParent();
+
+  InlineFunctionInfo FnInfo;
+  auto InlineRes = InlineFunction(*NewCall, FnInfo);
+  assert(InlineRes.isSuccess() && "Expected inlining to succeed");
+  (void)InlineRes;

ChuanqiXu9 wrote:

Don't do this. This should be done by later inlining pass. We prefer one pass 
do one thing strategy.

https://github.com/llvm/llvm-project/pull/79712
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

@@ -232,16 +237,59 @@ static LValueOrRValue 
emitSuspendExpression(CodeGenFunction , CGCoroData 
   auto *NullPtr = llvm::ConstantPointerNull::get(CGF.CGM.Int8PtrTy);
   auto *SaveCall = Builder.CreateCall(CoroSave, {NullPtr});
 
-  CGF.CurCoro.InSuspendBlock = true;
-  auto *SuspendRet = CGF.EmitScalarExpr(S.getSuspendExpr());
-  CGF.CurCoro.InSuspendBlock = false;
+  const auto AwaitSuspendCanThrow =
+  AwaitSuspendStmtCanThrow(S.getSuspendExpr());
+
+  auto SuspendHelper = CodeGenFunction(CGF.CGM).generateAwaitSuspendHelper(
+  CGF.CurFn->getName(), Prefix, S, AwaitSuspendCanThrow);
 
-  if (SuspendRet != nullptr && SuspendRet->getType()->isIntegerTy(1)) {
-// Veto suspension if requested by bool returning await_suspend.
-BasicBlock *RealSuspendBlock =
-CGF.createBasicBlock(Prefix + Twine(".suspend.bool"));
-CGF.Builder.CreateCondBr(SuspendRet, RealSuspendBlock, ReadyBlock);
-CGF.EmitBlock(RealSuspendBlock);
+  llvm::CallBase *SuspendRet = nullptr;
+
+  {
+CGF.CurCoro.InSuspendBlock = true;
+
+auto FramePtrBinder = CodeGenFunction::OpaqueValueMappingData::bind(
+CGF, S.getOpaqueFramePtr(), S.getOpaqueFramePtr()->getSourceExpr());
+auto UnbindFramePtrOnExit =
+llvm::make_scope_exit([&] { FramePtrBinder.unbind(CGF); });
+
+SmallVector SuspendHelperCallArgs;
+SuspendHelperCallArgs.push_back(
+
CGF.getOrCreateOpaqueLValueMapping(S.getOpaqueValue()).getPointer(CGF));
+SuspendHelperCallArgs.push_back(
+CGF.getOrCreateOpaqueRValueMapping(S.getOpaqueFramePtr())
+.getScalarVal());
+SuspendHelperCallArgs.push_back(SuspendHelper);
+
+auto IID = llvm::Intrinsic::coro_await_suspend;
+if (S.getSuspendExpr()->getType()->isBooleanType())
+  IID = llvm::Intrinsic::coro_await_suspend_bool;
+else if (S.getSuspendExpr()->getType()->isVoidPointerType())
+  IID = llvm::Intrinsic::coro_await_suspend_handle;

ChuanqiXu9 wrote:

nit: This pattern occurs multiple times. So maybe we can add an enum 
`SuspendReturnType` to `CoroutineSuspendExpr` and a getter for that. Then we 
can use a switch here to make it looks better.

https://github.com/llvm/llvm-project/pull/79712
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

@@ -1744,6 +1744,273 @@ a call to ``llvm.coro.suspend.retcon`` after resuming 
abnormally.
 In a yield-once coroutine, it is undefined behavior if the coroutine
 executes a call to ``llvm.coro.suspend.retcon`` after resuming in any way.
 
+.. _coro.await.suspend:
+
+'llvm.coro.await.suspend' Intrinsic
+^^
+::
+
+  declare void @llvm.coro.await.suspend(
+ptr ,
+ptr ,
+ptr )
+
+Overview:
+"
+
+The '``llvm.coro.await.suspend``' intrinsic hides C++ `await-suspend`
+block code from optimizations on presplit coroutine body 
+to avoid miscompilations. This version of intrinsic corresponds to 
+'``void awaiter.await_suspend(...)``' variant.
+
+Arguments:
+""
+
+The first argument is a pointer to `awaiter` object.
+
+The second argument is a pointer to the current coroutine's frame.
+
+The third argument is a pointer to the helper function encapsulating
+`await-suspend` logic. Its signature must be
+
+.. code-block:: llvm
+
+declare void @await_suspend_function(ptr %awaiter, ptr %hdl)
+
+Semantics:
+""
+
+The intrinsic must be used between corresponding `coro.save`_ and 
+`coro.suspend`_ calls. It is lowered to an inlined 
+`await_suspend_function` call during `CoroSplit`_ pass.
+
+Example:
+
+
+.. code-block:: llvm
+
+  ; before lowering
+  await.suspend:
+%save = call token @llvm.coro.save(ptr %hdl)
+call void @llvm.coro.await.suspend(
+ptr %awaiter,
+ptr %hdl,
+ptr @await_suspend_function)
+%suspend = call i8 @llvm.coro.suspend(token %save, i1 false)
+...
+
+  ; after lowering
+  await.suspend:
+%save = call token @llvm.coro.save(ptr %hdl)
+; the call to await_suspend_function is inlined
+call void @await_suspend_function(
+ptr %awaiter,
+ptr %hdl)
+%suspend = call i8 @llvm.coro.suspend(token %save, i1 false)   
+...
+
+  ; helper function example
+  define void @await_suspend_function(ptr %awaiter, ptr %hdl)
+entry:
+  %hdl.tmp = alloca %"struct.std::coroutine_handle"
+  %hdl.result.tmp = alloca %"struct.std::coroutine_handle"
+  %hdl.promise.tmp = alloca %"struct.std::coroutine_handle.0"
+  %hdl.promise = call ptr 
@"std::corouine_handle::from_address"(ptr %hdl)
+  %hdl.promise.tmp.dive = getelementptr inbounds 
%"struct.std::coroutine_handle.0",
+ptr %hdl.promise.tmp, i32 0, i32 0
+  %hdl.promise.tmp.dive2 = getelementptr inbounds 
%"struct.std::coroutine_handle",
+ptr %hdl.promise.tmp.dive, i32 0, i32 0
+  store ptr %hdl.promise, ptr %hdl.promise.tmp.dive2
+  call void @llvm.memcpy.p0.p0.i64(ptr %hdl.tmp, ptr %hdl.promise.tmp, i64 
8, i1 false)
+  %hdl.tmp.dive = getelementptr inbounds %"struct.std::coroutine_handle",
+ptr %hdl.tmp, i32 0, i32 0
+  %hdl.arg = load ptr, ptr %hdl.tmp.dive
+  call void @"Awaiter::await_suspend"(ptr %awaiter, ptr %hdl.arg)
+  ret void
+
+.. _coro.await.suspend.bool:
+
+'llvm.coro.await.suspend.bool' Intrinsic
+^^
+::
+
+  declare i1 @llvm.coro.await.suspend.bool(
+ptr ,
+ptr ,
+ptr )
+
+Overview:
+"
+
+The '``llvm.coro.await.suspend.bool``' intrinsic hides C++ `await-suspend`
+block code from optimizations on presplit coroutine body 
+to avoid miscompilations. This version of intrinsic corresponds to 
+'``bool awaiter.await_suspend(...)``' variant.
+
+Arguments:
+""
+
+The first argument is a pointer to `awaiter` object.
+
+The second argument is a pointer to the current coroutine's frame.
+
+The third argument is a pointer to the helper function encapsulating
+`await-suspend` logic. Its signature must be
+
+.. code-block:: llvm
+
+declare i1 @await_suspend_function(ptr %awaiter, ptr %hdl)
+
+Semantics:
+""
+
+The intrinsic must be used between corresponding `coro.save`_ and 
+`coro.suspend`_ calls. It is lowered to an inlined 
+`await_suspend_function` call during `CoroSplit`_ pass.
+
+If `await_suspend_function` call returns `true`, the current coroutine is
+immediately resumed.
+
+Example:
+
+
+.. code-block:: llvm
+
+  ; before lowering
+  await.suspend:
+%save = call token @llvm.coro.save(ptr %hdl)
+%resume = call i1 @llvm.coro.await.suspend(
+ptr %awaiter,
+ptr %hdl,
+ptr @await_suspend_function)
+br i1 %resume, %await.suspend.bool, %await.ready
+  await.suspend.bool:
+%suspend = call i8 @llvm.coro.suspend(token %save, i1 false)
+...
+  await.ready:
+call void @"Awaiter::await_ready"(ptr %awaiter)

ChuanqiXu9 wrote:

shouldn't this be `await_resume`? await_ready should happen before await_suspend

https://github.com/llvm/llvm-project/pull/79712
___
cfe-commits mailing list

[llvm] [clang] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

@@ -1744,6 +1744,273 @@ a call to ``llvm.coro.suspend.retcon`` after resuming 
abnormally.
 In a yield-once coroutine, it is undefined behavior if the coroutine
 executes a call to ``llvm.coro.suspend.retcon`` after resuming in any way.
 
+.. _coro.await.suspend:
+
+'llvm.coro.await.suspend' Intrinsic
+^^
+::
+
+  declare void @llvm.coro.await.suspend(
+ptr ,
+ptr ,
+ptr )
+
+Overview:
+"
+
+The '``llvm.coro.await.suspend``' intrinsic hides C++ `await-suspend`
+block code from optimizations on presplit coroutine body 
+to avoid miscompilations. This version of intrinsic corresponds to 
+'``void awaiter.await_suspend(...)``' variant.
+
+Arguments:
+""
+
+The first argument is a pointer to `awaiter` object.
+
+The second argument is a pointer to the current coroutine's frame.
+
+The third argument is a pointer to the helper function encapsulating
+`await-suspend` logic. Its signature must be
+
+.. code-block:: llvm
+
+declare void @await_suspend_function(ptr %awaiter, ptr %hdl)
+
+Semantics:
+""
+
+The intrinsic must be used between corresponding `coro.save`_ and 
+`coro.suspend`_ calls. It is lowered to an inlined 
+`await_suspend_function` call during `CoroSplit`_ pass.
+
+Example:
+
+
+.. code-block:: llvm
+
+  ; before lowering
+  await.suspend:
+%save = call token @llvm.coro.save(ptr %hdl)
+call void @llvm.coro.await.suspend(
+ptr %awaiter,
+ptr %hdl,
+ptr @await_suspend_function)
+%suspend = call i8 @llvm.coro.suspend(token %save, i1 false)
+...
+
+  ; after lowering
+  await.suspend:
+%save = call token @llvm.coro.save(ptr %hdl)
+; the call to await_suspend_function is inlined
+call void @await_suspend_function(
+ptr %awaiter,
+ptr %hdl)
+%suspend = call i8 @llvm.coro.suspend(token %save, i1 false)   
+...
+
+  ; helper function example
+  define void @await_suspend_function(ptr %awaiter, ptr %hdl)
+entry:
+  %hdl.tmp = alloca %"struct.std::coroutine_handle"
+  %hdl.result.tmp = alloca %"struct.std::coroutine_handle"
+  %hdl.promise.tmp = alloca %"struct.std::coroutine_handle.0"
+  %hdl.promise = call ptr 
@"std::corouine_handle::from_address"(ptr %hdl)
+  %hdl.promise.tmp.dive = getelementptr inbounds 
%"struct.std::coroutine_handle.0",
+ptr %hdl.promise.tmp, i32 0, i32 0
+  %hdl.promise.tmp.dive2 = getelementptr inbounds 
%"struct.std::coroutine_handle",
+ptr %hdl.promise.tmp.dive, i32 0, i32 0
+  store ptr %hdl.promise, ptr %hdl.promise.tmp.dive2
+  call void @llvm.memcpy.p0.p0.i64(ptr %hdl.tmp, ptr %hdl.promise.tmp, i64 
8, i1 false)
+  %hdl.tmp.dive = getelementptr inbounds %"struct.std::coroutine_handle",
+ptr %hdl.tmp, i32 0, i32 0
+  %hdl.arg = load ptr, ptr %hdl.tmp.dive
+  call void @"Awaiter::await_suspend"(ptr %awaiter, ptr %hdl.arg)
+  ret void
+
+.. _coro.await.suspend.bool:
+
+'llvm.coro.await.suspend.bool' Intrinsic
+^^
+::
+
+  declare i1 @llvm.coro.await.suspend.bool(
+ptr ,
+ptr ,
+ptr )
+
+Overview:
+"
+
+The '``llvm.coro.await.suspend.bool``' intrinsic hides C++ `await-suspend`
+block code from optimizations on presplit coroutine body 
+to avoid miscompilations. This version of intrinsic corresponds to 
+'``bool awaiter.await_suspend(...)``' variant.
+
+Arguments:
+""
+
+The first argument is a pointer to `awaiter` object.
+
+The second argument is a pointer to the current coroutine's frame.
+
+The third argument is a pointer to the helper function encapsulating
+`await-suspend` logic. Its signature must be
+
+.. code-block:: llvm
+
+declare i1 @await_suspend_function(ptr %awaiter, ptr %hdl)
+
+Semantics:
+""
+
+The intrinsic must be used between corresponding `coro.save`_ and 
+`coro.suspend`_ calls. It is lowered to an inlined 
+`await_suspend_function` call during `CoroSplit`_ pass.
+
+If `await_suspend_function` call returns `true`, the current coroutine is
+immediately resumed.
+
+Example:
+
+
+.. code-block:: llvm
+
+  ; before lowering
+  await.suspend:
+%save = call token @llvm.coro.save(ptr %hdl)
+%resume = call i1 @llvm.coro.await.suspend(
+ptr %awaiter,
+ptr %hdl,
+ptr @await_suspend_function)
+br i1 %resume, %await.suspend.bool, %await.ready
+  await.suspend.bool:
+%suspend = call i8 @llvm.coro.suspend(token %save, i1 false)
+...
+  await.ready:
+call void @"Awaiter::await_ready"(ptr %awaiter)
+...
+
+  ; after lowering
+  await.suspend:
+%save = call token @llvm.coro.save(ptr %hdl)
+; the call to await_suspend_function is inlined
+%resume = call i1 @await_suspend_function(
+ptr %awaiter,
+ptr %hdl)
+ 

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

@@ -232,16 +237,59 @@ static LValueOrRValue 
emitSuspendExpression(CodeGenFunction , CGCoroData 
   auto *NullPtr = llvm::ConstantPointerNull::get(CGF.CGM.Int8PtrTy);
   auto *SaveCall = Builder.CreateCall(CoroSave, {NullPtr});
 
-  CGF.CurCoro.InSuspendBlock = true;
-  auto *SuspendRet = CGF.EmitScalarExpr(S.getSuspendExpr());
-  CGF.CurCoro.InSuspendBlock = false;
+  const auto AwaitSuspendCanThrow =
+  AwaitSuspendStmtCanThrow(S.getSuspendExpr());
+
+  auto SuspendHelper = CodeGenFunction(CGF.CGM).generateAwaitSuspendHelper(
+  CGF.CurFn->getName(), Prefix, S, AwaitSuspendCanThrow);
 
-  if (SuspendRet != nullptr && SuspendRet->getType()->isIntegerTy(1)) {
-// Veto suspension if requested by bool returning await_suspend.
-BasicBlock *RealSuspendBlock =
-CGF.createBasicBlock(Prefix + Twine(".suspend.bool"));
-CGF.Builder.CreateCondBr(SuspendRet, RealSuspendBlock, ReadyBlock);
-CGF.EmitBlock(RealSuspendBlock);
+  llvm::CallBase *SuspendRet = nullptr;
+

ChuanqiXu9 wrote:

We need comment here about the emitted code.  Originally this is commented in 
the above the function but I can't inline comments that due the review system 
of GH...

https://github.com/llvm/llvm-project/pull/79712
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

@@ -1744,6 +1744,273 @@ a call to ``llvm.coro.suspend.retcon`` after resuming 
abnormally.
 In a yield-once coroutine, it is undefined behavior if the coroutine
 executes a call to ``llvm.coro.suspend.retcon`` after resuming in any way.
 
+.. _coro.await.suspend:
+
+'llvm.coro.await.suspend' Intrinsic

ChuanqiXu9 wrote:

```suggestion
'llvm.coro.await.suspend_void' Intrinsic
```

I'd like to rename it to keep consistency. nit

https://github.com/llvm/llvm-project/pull/79712
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

@@ -79,6 +79,73 @@ using namespace llvm;
 
 namespace {
 
+// Created on demand if the coro-early pass has work to do.
+class Lowerer : public coro::LowererBase {
+  IRBuilder<> Builder;
+  void lowerAwaitSuspend(CoroAwaitSuspendInst *CB);
+
+public:
+  Lowerer(Module ) : LowererBase(M), Builder(Context) {}
+
+  void lowerAwaitSuspends(Function );
+};
+
+void Lowerer::lowerAwaitSuspend(CoroAwaitSuspendInst *CB) {
+  auto Helper = CB->getHelperFunction();
+  auto Awaiter = CB->getAwaiter();
+  auto FramePtr = CB->getFrame();
+
+  Builder.SetInsertPoint(CB);
+
+  CallBase *NewCall = nullptr;
+  if (auto Invoke = dyn_cast(CB)) {
+auto HelperInvoke =
+Builder.CreateInvoke(Helper, Invoke->getNormalDest(),
+ Invoke->getUnwindDest(), {Awaiter, FramePtr});
+
+HelperInvoke->setCallingConv(Invoke->getCallingConv());
+std::copy(Invoke->bundle_op_info_begin(), Invoke->bundle_op_info_end(),
+  HelperInvoke->bundle_op_info_begin());
+AttributeList NewAttributes =
+Invoke->getAttributes().removeParamAttributes(Context, 2);
+HelperInvoke->setAttributes(NewAttributes);
+HelperInvoke->setDebugLoc(Invoke->getDebugLoc());
+NewCall = HelperInvoke;
+  } else if (auto Call = dyn_cast(CB)) {
+auto HelperCall = Builder.CreateCall(Helper, {Awaiter, FramePtr});
+
+AttributeList NewAttributes =
+Call->getAttributes().removeParamAttributes(Context, 2);
+HelperCall->setAttributes(NewAttributes);
+HelperCall->setDebugLoc(Call->getDebugLoc());
+NewCall = HelperCall;
+  }

ChuanqiXu9 wrote:

```suggestion
  } else
  llvm_unreachable(...)
```

nit: personal interest

https://github.com/llvm/llvm-project/pull/79712
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

@@ -5036,14 +5036,17 @@ class CoroutineSuspendExpr : public Expr {
 
   Stmt *SubExprs[SubExpr::Count];
   OpaqueValueExpr *OpaqueValue = nullptr;
+  OpaqueValueExpr *OpaqueFramePtr = nullptr;

ChuanqiXu9 wrote:

I still think we can get rid of storing `OpaqueFramePtr` in 
`CoroutineSuspendExpr`. For example, we can call ` @llvm.coro.frame()` directly 
to get it. 

https://llvm.org/docs/Coroutines.html#llvm-coro-frame-intrinsic

https://github.com/llvm/llvm-project/pull/79712
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

@@ -232,16 +237,59 @@ static LValueOrRValue 
emitSuspendExpression(CodeGenFunction , CGCoroData 
   auto *NullPtr = llvm::ConstantPointerNull::get(CGF.CGM.Int8PtrTy);
   auto *SaveCall = Builder.CreateCall(CoroSave, {NullPtr});
 
-  CGF.CurCoro.InSuspendBlock = true;
-  auto *SuspendRet = CGF.EmitScalarExpr(S.getSuspendExpr());
-  CGF.CurCoro.InSuspendBlock = false;
+  const auto AwaitSuspendCanThrow =
+  AwaitSuspendStmtCanThrow(S.getSuspendExpr());
+
+  auto SuspendHelper = CodeGenFunction(CGF.CGM).generateAwaitSuspendHelper(
+  CGF.CurFn->getName(), Prefix, S, AwaitSuspendCanThrow);
 
-  if (SuspendRet != nullptr && SuspendRet->getType()->isIntegerTy(1)) {
-// Veto suspension if requested by bool returning await_suspend.
-BasicBlock *RealSuspendBlock =
-CGF.createBasicBlock(Prefix + Twine(".suspend.bool"));
-CGF.Builder.CreateCondBr(SuspendRet, RealSuspendBlock, ReadyBlock);
-CGF.EmitBlock(RealSuspendBlock);
+  llvm::CallBase *SuspendRet = nullptr;
+
+  {
+CGF.CurCoro.InSuspendBlock = true;
+
+auto FramePtrBinder = CodeGenFunction::OpaqueValueMappingData::bind(
+CGF, S.getOpaqueFramePtr(), S.getOpaqueFramePtr()->getSourceExpr());
+auto UnbindFramePtrOnExit =
+llvm::make_scope_exit([&] { FramePtrBinder.unbind(CGF); });
+
+SmallVector SuspendHelperCallArgs;
+SuspendHelperCallArgs.push_back(
+
CGF.getOrCreateOpaqueLValueMapping(S.getOpaqueValue()).getPointer(CGF));
+SuspendHelperCallArgs.push_back(
+CGF.getOrCreateOpaqueRValueMapping(S.getOpaqueFramePtr())
+.getScalarVal());
+SuspendHelperCallArgs.push_back(SuspendHelper);
+
+auto IID = llvm::Intrinsic::coro_await_suspend;
+if (S.getSuspendExpr()->getType()->isBooleanType())
+  IID = llvm::Intrinsic::coro_await_suspend_bool;
+else if (S.getSuspendExpr()->getType()->isVoidPointerType())
+  IID = llvm::Intrinsic::coro_await_suspend_handle;
+
+llvm::Function *AwaitSuspendIntrinsic = CGF.CGM.getIntrinsic(IID);
+// FIXME: add call attributes?
+if (AwaitSuspendCanThrow)
+  SuspendRet =
+  CGF.EmitCallOrInvoke(AwaitSuspendIntrinsic, SuspendHelperCallArgs);
+else
+  SuspendRet = CGF.EmitNounwindRuntimeCall(AwaitSuspendIntrinsic,
+   SuspendHelperCallArgs);
+
+CGF.CurCoro.InSuspendBlock = false;
+  }
+
+  if (SuspendRet != nullptr) {
+if (SuspendRet->getType()->isIntegerTy(1)) {
+  // Veto suspension if requested by bool returning await_suspend.
+  BasicBlock *RealSuspendBlock =
+  CGF.createBasicBlock(Prefix + Twine(".suspend.bool"));
+  CGF.Builder.CreateCondBr(SuspendRet, RealSuspendBlock, ReadyBlock);
+  CGF.EmitBlock(RealSuspendBlock);
+} else if (SuspendRet->getType()->isPointerTy()) {
+  auto ResumeIntrinsic = 
CGF.CGM.getIntrinsic(llvm::Intrinsic::coro_resume);
+  Builder.CreateCall(ResumeIntrinsic, SuspendRet);
+}

ChuanqiXu9 wrote:

```suggestion
} else
 llvm_unreachable(...);
```

nit: personal interest.

https://github.com/llvm/llvm-project/pull/79712
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

@@ -173,6 +173,10 @@ static bool ResumeStmtCanThrow(const Stmt *S) {
   return false;
 }
 
+static bool AwaitSuspendStmtCanThrow(const Stmt *S) {
+  return ResumeStmtCanThrow(S);
+}

ChuanqiXu9 wrote:

Maybe it will be better to rename `ResumeStmtCanThrow` to `StmtCanThrow`

https://github.com/llvm/llvm-project/pull/79712
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 commented:

I got the reason why I felt `__await_suspend_helper_` was odd. In my 
imagination, we only need to emit the function `await_suspend(handle)` to an 
LLVM function and pass that to `llvm.coro.await.suspend` directly.

https://github.com/llvm/llvm-project/pull/79712
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 edited 
https://github.com/llvm/llvm-project/pull/79712
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> I'd still idly vote against adding this flag/support - but if other modules 
> contributors feel it's the right thing to do, I won't stand in the way.

Yeah, as @mizvekov said, this is intended to be a transparent change to users. 
(unless the users are testing volunteers, which is highly welcomed)

https://github.com/llvm/llvm-project/pull/79959
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Chuanqi Xu via cfe-commits]

@@ -457,6 +457,28 @@ Note that **currently** the compiler doesn't consider 
inconsistent macro definit
 Currently Clang would accept the above example. But it may produce surprising 
results if the
 debugging code depends on consistent use of ``NDEBUG`` also in other 
translation units.
 
+Definitions consistency
+^^^
+
+The C++ language defines that same declarations in different translation units 
should have
+the same definition, as known as ODR (One Definition Rule). Prior to modules, 
the translation
+units don't dependent on each other and the compiler itself don't and can't 
perform a strong
+ODR violation check. Sometimes it is the linker does some jobs related to ODR, 
where the
+higher level semantics are missing. With the introduction of modules, now the 
compiler have
+the chance to perform ODR violations with language semantics across 
translation units.
+
+However, in the practice we found the existing ODR checking mechanism may be 
too aggressive.
+In the many issue reports about ODR violation diagnostics, most of them are 
false positive
+ODR violations and the true positive ODR violations are rarely reported. Also 
MSVC don't
+perform ODR check for declarations in the global module fragment.
+
+So in order to get better user experience, save the time checking ODR and keep 
consistent
+behavior with MSVC, we disabled the ODR check for the declarations in the 
global module
+fragment by default. Users who want more strict check can still use the
+``-Xclang -fno-skip-odr-check-in-gmf`` flag to get the ODR check enabled. It 
is also
+encouraged to report issues if users find false positive ODR violations or 
false negative ODR
+violations with the flag enabled.

ChuanqiXu9 wrote:

My thought for writing this paragraph is:
1. The most important reason is to get better user experience. This is the 
major and the most important motivation.
2. Saving compilation time is the benefit we got after making this decision. 
And we can mention it to say something is getting better in some degree and 
under some conditions (the existing ODR checker is not good enough).
3. Keep consistent behavior with MSVC. This is a supporting argument. And we 
can find that this is the least important thing from the order. This shouldn't 
be a blocker if someday we think the ODR checker is good enough and want to 
enable the ODR checker by default.

I can understand your concerning totally. This is not a document for teaching 
developers. This document is for users. I feel it will be better to write the 
document in the current way to make the users to understand what's happening.

https://github.com/llvm/llvm-project/pull/79959
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Chuanqi Xu via cfe-commits]

@@ -457,6 +457,28 @@ Note that **currently** the compiler doesn't consider 
inconsistent macro definit
 Currently Clang would accept the above example. But it may produce surprising 
results if the
 debugging code depends on consistent use of ``NDEBUG`` also in other 
translation units.
 
+Definitions consistency
+^^^
+
+The C++ language defines that same declarations in different translation units 
should have
+the same definition, as known as ODR (One Definition Rule). Prior to modules, 
the translation
+units don't dependent on each other and the compiler itself don't and can't 
perform a strong
+ODR violation check. Sometimes it is the linker does some jobs related to ODR, 
where the
+higher level semantics are missing. With the introduction of modules, now the 
compiler have
+the chance to perform ODR violations with language semantics across 
translation units.
+
+However, in the practice we found the existing ODR checking mechanism may be 
too aggressive.
+In the many issue reports about ODR violation diagnostics, most of them are 
false positive
+ODR violations and the true positive ODR violations are rarely reported. Also 
MSVC don't
+perform ODR check for declarations in the global module fragment.

ChuanqiXu9 wrote:

For 1, I don't understand why it is relevant here. I know what you're saying, 
but I just feel it is not related here. We're talking about ODR checker, right?

For 2, I thought the term `false positive` implies 2. But maybe it is not 
impressive. So I tried to rewrite this paragraph to make it more explicit.

For MSVC, it is a simple supporting argument here. I mean, the document is for 
users, and what I want to say or express is, while all of us know and 
understand it is not good, but you (the users) don't need to be too panic since 
MSVC did the same thing.

BTW, for the delayed template parsing, I remember one of the supporting reason 
to disable that in C++20 is that MSVC disable that in C++20 too. But this is 
not relevant here.

https://github.com/llvm/llvm-project/pull/79959
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 updated 
https://github.com/llvm/llvm-project/pull/79959

>From beb1a4b89f941f41a6e220447dcda6d6fc231a0b Mon Sep 17 00:00:00 2001
From: Chuanqi Xu 
Date: Tue, 30 Jan 2024 15:57:35 +0800
Subject: [PATCH 1/2] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf

Close https://github.com/llvm/llvm-project/issues/79240

Cite the comment from @mizvekov in
//github.com/llvm/llvm-project/issues/79240:

> There are two kinds of bugs / issues relevant here:
>
> Clang bugs that this change hides
> Here we can add a Frontend flag that disables the GMF ODR check, just
> so
> we can keep tracking, testing and fixing these issues.
> The Driver would just always pass that flag.
> We could add that flag in this current issue.
> Bugs in user code:
> I don't think it's worth adding a corresponding Driver flag for
> controlling the above Frontend flag, since we intend it's behavior to
> become default as we fix the problems, and users interested in testing
> the more strict behavior can just use the Frontend flag directly.

This patch follows the suggestion:
- Introduce the CC1 flag `-fskip-odr-check-in-gmf` which is by default
  off,
  so that the every existing test will still be tested with checking ODR
  violations.
- Passing `-fskip-odr-check-in-gmf` in the driver to keep the behavior
  we intended.
- Edit the document to tell the users who are still interested in more
  strict checks can use `-Xclang -fno-skip-odr-check-in-gmf` to get the
  existing behavior.
---
 clang/docs/ReleaseNotes.rst   |  3 +-
 clang/docs/StandardCPlusPlusModules.rst   | 22 
 clang/include/clang/Basic/LangOptions.def |  1 +
 clang/include/clang/Driver/Options.td |  8 +++
 clang/include/clang/Serialization/ASTReader.h |  6 +-
 clang/lib/Driver/ToolChains/Clang.cpp |  8 +++
 clang/lib/Serialization/ASTReader.cpp |  2 +-
 clang/lib/Serialization/ASTReaderDecl.cpp | 17 +++---
 clang/lib/Serialization/ASTWriter.cpp |  2 +-
 clang/lib/Serialization/ASTWriterDecl.cpp |  8 +--
 .../Driver/modules-skip-odr-check-in-gmf.cpp  | 10 
 clang/test/Modules/concept.cppm   | 23 +---
 clang/test/Modules/polluted-operator.cppm | 18 +-
 clang/test/Modules/pr76638.cppm   | 16 +-
 clang/test/Modules/skip-odr-check-in-gmf.cppm | 56 +++
 15 files changed, 170 insertions(+), 30 deletions(-)
 create mode 100644 clang/test/Driver/modules-skip-odr-check-in-gmf.cpp
 create mode 100644 clang/test/Modules/skip-odr-check-in-gmf.cppm

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index bac6c7162a45b..74f34eeef65f0 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -64,7 +64,8 @@ C++20 Feature Support
 
 - Clang won't perform ODR checks for decls in the global module fragment any
   more to ease the implementation and improve the user's using experience.
-  This follows the MSVC's behavior.
+  This follows the MSVC's behavior. Users interested in testing the more strict
+  behavior can use the flag '-Xclang -fno-skip-odr-check-in-gmf'.
   (`#79240 `_).
 
 C++23 Feature Support
diff --git a/clang/docs/StandardCPlusPlusModules.rst 
b/clang/docs/StandardCPlusPlusModules.rst
index 4e853990a7338..44870f210cde6 100644
--- a/clang/docs/StandardCPlusPlusModules.rst
+++ b/clang/docs/StandardCPlusPlusModules.rst
@@ -457,6 +457,28 @@ Note that **currently** the compiler doesn't consider 
inconsistent macro definit
 Currently Clang would accept the above example. But it may produce surprising 
results if the
 debugging code depends on consistent use of ``NDEBUG`` also in other 
translation units.
 
+Definitions consistency
+^^^
+
+The C++ language defines that same declarations in different translation units 
should have
+the same definition, as known as ODR (One Definition Rule). Prior to modules, 
the translation
+units don't dependent on each other and the compiler itself don't and can't 
perform a strong
+ODR violation check. Sometimes it is the linker does some jobs related to ODR, 
where the
+higher level semantics are missing. With the introduction of modules, now the 
compiler have
+the chance to perform ODR violations with language semantics across 
translation units.
+
+However, in the practice we found the existing ODR checking mechanism may be 
too aggressive.
+In the many issue reports about ODR violation diagnostics, most of them are 
false positive
+ODR violations and the true positive ODR violations are rarely reported. Also 
MSVC don't
+perform ODR check for declarations in the global module fragment.
+
+So in order to get better user experience, save the time checking ODR and keep 
consistent
+behavior with MSVC, we disabled the ODR check for the declarations in the 
global module
+fragment by default. Users who want more strict check can still use the
+``-Xclang -fno-skip-odr-check-in-gmf`` 

[clang] Support C++20 Modules in clang-repl (PR #79261)

[Chuanqi Xu via cfe-commits]

@@ -0,0 +1,31 @@
+// UNSUPPORTED: system-aix
+//
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang -std=c++20 %t/mod.cppm --precompile \
+// RUN: -o %t/mod.pcm
+// RUN: %clang %t/mod.pcm -c -o %t/mod.o
+// RUN: %clang -shared %t/mod.o -o %t/libmod.so
+//
+// RUN: cat %t/import.cpp | env LD_LIBRARY_PATH=%t:$LD_LIBRARY_PATH \
+// RUN: clang-repl -Xcc=-std=c++20 -Xcc=-fmodule-file=M=%t/mod.pcm \
+// RUN: | FileCheck %t/import.cpp

ChuanqiXu9 wrote:

@nathanchance this is what I expected to test

https://github.com/llvm/llvm-project/pull/79261
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Support C++20 Modules in clang-repl (PR #79261)

[Chuanqi Xu via cfe-commits]

@@ -0,0 +1,31 @@
+// UNSUPPORTED: system-aix
+//
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang -std=c++20 %t/mod.cppm --precompile \
+// RUN: -o %t/mod.pcm
+// RUN: %clang %t/mod.pcm -c -o %t/mod.o
+// RUN: %clang -shared %t/mod.o -o %t/libmod.so
+//
+// RUN: cat %t/import.cpp | env LD_LIBRARY_PATH=%t:$LD_LIBRARY_PATH \
+// RUN: clang-repl -Xcc=-std=c++20 -Xcc=-fmodule-file=M=%t/mod.pcm \
+// RUN: | FileCheck %t/import.cpp

ChuanqiXu9 wrote:

// RUN: %clang -std=c++20 %t/mod.cppm --precompile \
// RUN: -o %t/mod.pcm -triple=x86_64-linux-gnu
// RUN: %clang %t/mod.pcm -c -o %t/mod.o -triple=x86_64-linux-gnu
// RUN: %clang -shared %t/mod.o -o %t/libmod.so -triple=x86_64-linux-gnu
//
// RUN: cat %t/import.cpp | env LD_LIBRARY_PATH=%t:$LD_LIBRARY_PATH \
// RUN: clang-repl -Xcc=-std=c++20 -Xcc=-fmodule-file=M=%t/mod.pcm \
// RUN: -Xcc=-triple=x86_64-linux-gnu | FileCheck %t/import.cpp

https://github.com/llvm/llvm-project/pull/79261
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Support C++20 Modules in clang-repl (PR #79261)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 commented:

This is what I had in mind

https://github.com/llvm/llvm-project/pull/79261
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Support C++20 Modules in clang-repl (PR #79261)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 edited 
https://github.com/llvm/llvm-project/pull/79261
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-scan-deps] Fix check for empty `Compilation` (PR #75545)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 closed 
https://github.com/llvm/llvm-project/pull/75545
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-scan-deps] Fix check for empty `Compilation` (PR #75545)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 approved this pull request.

LGTM then.

https://github.com/llvm/llvm-project/pull/75545
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Check for stack exhaustion when reading declarations (PR #79875)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> with stack exhaustion warning, the compiler can continue (albeit being slow 
> or unstable). The depth limit here will be a hard restriction and so there 
> will be no workaround if the code reaches it.

It is a surprise to me that this is only a warning instead of a hard error. I 
never thought it can continue when the stack runs out. Sorry for confusing.

> I am curious about performance concerns. I feel that the amount of work in 
> ASTReaderDecl is large enough that this extra call should not add any 
> noticeable overhead on a happy path (when we have enough stack). Or do you 
> think the overhead might be problematic?

In my imagination, with `ASTReader::NumCurrentElementsDeserializing? will only 
require us to insert a compare between intergers (the value of 
`NumCurrentElementsDeserializing` and the threshold) on the hot path. So it 
will be simpler than the current method. But given you said above, maybe it 
doesn't matter so much. I believe the overhead is under control.

> Except the error should ideally be happening when we write PCM, but that 
> seems hard

What's the meaning? Isn't this patch about reading?

> Just to make sure I got everything right this time. So you mean testing the 
> clang::runWithSufficientStackSpace itself? That seems very reasonable. If 
> we're talking about unit-testing the ASTReaderDecl, this seems like it's 
> quite a bit of work (too much setup code that needs to be put around it).

I mean something like:

First, refactor the signature of `clang::runWithSufficientStackSpace` to:


```
namespace clang {
clang::runWithSufficientStackSpace(function_call_back_for_diagnostic, 
function_to_run, desired_stack_size = default_value, sufficient_stack_size = 
default_value);
}
```

then add two setters to ASTReader

```
class ASTReader {
std::optional desired_stack_size;
std::optional sufficient_stack_size;
public:
 void set_desired_stack_size(size_t) {...}
 void set_sufficient_stack_size(size_t) {...}
}
```

then in the unittest under `clang/unittests/Serialization`, we can generate the 
BMI for the codes in your current patch (but with smaller scale) like other 
unit test did.

And we can try to parse the `use.cpp` in the current patch, we need to 
construct the CompilerInstance and get the ASTReader and set the corresponding 
value for desired_stack_size and sufficient_stack_size. Since we're calling 
`clang::runWithSufficientStackSpace` with a call back to the diagnostic 
function, it is easy to test the diagnostic function is called or it shouldn't 
be hard to test the actual diagnostic message.

This is the code in my mind. I don't feel it would be pretty hard. How do you 
think about it?



https://github.com/llvm/llvm-project/pull/79875
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Check for stack exhaustion when reading declarations (PR #79875)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> I am not saying it's a bad idea to add this for testing purposes, but given 
> how fragile this approach is, I think we should not provide configuration 
> knobs to users there. At least everyone sees crashes at roughly the same 
> points right now (although variation is still present because depending on 
> inlining decisions stacks sizes might vary even with the same code and same 
> limits).

Sorry for not being clear enough. What I want is to add parameters to 
`clang::runWithSufficientStackSpace` and we can invoke it and test this in 
unittest. It won't be visible to users.

BTW, how about the idea of using ASTReader::NumCurrentElementsDeserializing?


https://github.com/llvm/llvm-project/pull/79875
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Check for stack exhaustion when reading declarations (PR #79875)

[Chuanqi Xu via cfe-commits]

@@ -4099,7 +4099,9 @@ Decl *ASTReader::ReadDeclRecord(DeclID ID) {
   // calls to Decl::getASTContext() by Decl's methods will find the
   // TranslationUnitDecl without crashing.
   D->setDeclContext(Context.getTranslationUnitDecl());
-  Reader.Visit(D);
+
+  // Reading some declarations can result in deep recursion.
+  SemaObj->runWithSufficientStackSpace(DeclLoc, [&] { Reader.Visit(D); });

ChuanqiXu9 wrote:

As I said, for example, if we're compiling a pcm file to an object file. It 
makes sense that the Sema is not needed here.

https://github.com/llvm/llvm-project/pull/79875
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Support C++20 Modules in clang-repl (PR #79261)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> > > IncrementalExtensions
> > 
> > 
> > But the change itself is to make `IncrementalExtensions` a compatible lang 
> > options. So that we don't need to specify it when generating the modules.
> 
> I mean conditionally if `IncrementalExtensions` is set we change the target 
> triple in the clang fork of the C++ module build.

I am not sure if it is possible since we don't know the clang fork before we 
see the import. Even if we can, it is more or less problematic if there are 
multiple imported module from different fork.

> 
> > I am wondering if it helps if we specify the target triple in the test, 
> > then it won't be so troublesome.
> 
> I was wondering that too. Can we express that in lit?

For `%clang_cc1`, we can use `%clang_cc1 -triple=x86_64-linux-gnu`. I am not 
sure about clang-repl. It may be possible by `-Xcc=`.

But due to I can't reproduce the failure, @nathanchance would you like to make 
this? I don't want to make something that I can't test.

https://github.com/llvm/llvm-project/pull/79261
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Support C++20 Modules in clang-repl (PR #79261)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

I am wondering if it helps if we specify the target triple in the test, then it 
won't be so troublesome.

https://github.com/llvm/llvm-project/pull/79261
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Support C++20 Modules in clang-repl (PR #79261)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> IncrementalExtensions

But the change itself is to make `IncrementalExtensions` a compatible lang 
options. So that we don't need to specify it when generating the modules.

https://github.com/llvm/llvm-project/pull/79261
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Load Specializations Lazily (PR #76774)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> > > > > As far as I can tell from [#76774 
> > > > > (comment)](https://github.com/llvm/llvm-project/pull/76774#issuecomment-1914177330)
> > > > >  above, the last push only changed the default value of 
> > > > > `LoadExternalSpecializationsLazily`. In that case, my test results 
> > > > > from [#76774 
> > > > > (comment)](https://github.com/llvm/llvm-project/pull/76774#issuecomment-1912182171)
> > > > >  remain fully valid, especially
> > > > > > Switching the new `LoadExternalSpecializationsLazily` to disabled 
> > > > > > by default (somehow the argument didn't work on its own) instead 
> > > > > > crashes, most of the cases involving `MultiOnDiskHashTable`. I 
> > > > > > suspect some kind of memory error maybe?
> > > > 
> > > > 
> > > > No, this newest patch changes besides changing the default value. For 
> > > > example, 
> > > > [22c9d11#diff-125f472e690aa3d973bc42aa3c5d580226c5c47661551aca2889f960681aa64dR232-R237](https://github.com/llvm/llvm-project/commit/22c9d1145eb57d9c2cb2ef490b7c474598dd5d12#diff-125f472e690aa3d973bc42aa3c5d580226c5c47661551aca2889f960681aa64dR232-R237)
> > > >  now we won't write specializations into the hash table, while the 
> > > > original patch tries to load all the decls from the consumers' side 
> > > > only. This patch touches the producer's side.
> > > > Maybe I shouldn't push force to make the history clear..
> > > 
> > > 
> > > If you want I can set you up with the ROOT project so that you can debug 
> > > the failures easier.
> > 
> > 
> > Thanks. Although I don't understand how can it be, it will be better if 
> > things can be easier.
> 
> ```shell
> git clone https://github.com/vgvassilev/root.git
> git checkout llvm-pr76774
> 
> mkdir root_obj && cd root_obj
> cmake -DCMAKE_BUILD_TYPE=Debug -DLLVM_BUILD_TYPE=Debug ../root
> ```
> 
> That will get you llvm-16 under src/interpreter/llvm-project. I have removed 
> our local patch that's essentially https://reviews.llvm.org/D41416 and 
> https://reviews.llvm.org/D154328. Then you will need to apply the changes 
> from this PR.
> 
> You might need to install some "trivial" pre-requirements 
> https://root.cern/install/dependencies/
> 
> Very likely a Clang-based tool called rootcling will fail with an error. You 
> can re-run with VERBOSE=1 and debug there.
> 
> I hope that's not too difficult and will help us narrow down the problems.

I made that but I find `ctest . -j` doesn't run any thing on that. So I tried 
to add `-Dbuiltin_gtest=ON -Dclingtest=ON -D roottest=ON -Dtesting=ON`.  Then I 
am able to run `ctest . -j96`. But I find the tests are failing. It is on 
baseline repo. I didn't change anything meaningful. Do you know what's wrong 
here?

https://github.com/llvm/llvm-project/pull/76774
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 updated 
https://github.com/llvm/llvm-project/pull/79959

>From beb1a4b89f941f41a6e220447dcda6d6fc231a0b Mon Sep 17 00:00:00 2001
From: Chuanqi Xu 
Date: Tue, 30 Jan 2024 15:57:35 +0800
Subject: [PATCH] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf

Close https://github.com/llvm/llvm-project/issues/79240

Cite the comment from @mizvekov in
//github.com/llvm/llvm-project/issues/79240:

> There are two kinds of bugs / issues relevant here:
>
> Clang bugs that this change hides
> Here we can add a Frontend flag that disables the GMF ODR check, just
> so
> we can keep tracking, testing and fixing these issues.
> The Driver would just always pass that flag.
> We could add that flag in this current issue.
> Bugs in user code:
> I don't think it's worth adding a corresponding Driver flag for
> controlling the above Frontend flag, since we intend it's behavior to
> become default as we fix the problems, and users interested in testing
> the more strict behavior can just use the Frontend flag directly.

This patch follows the suggestion:
- Introduce the CC1 flag `-fskip-odr-check-in-gmf` which is by default
  off,
  so that the every existing test will still be tested with checking ODR
  violations.
- Passing `-fskip-odr-check-in-gmf` in the driver to keep the behavior
  we intended.
- Edit the document to tell the users who are still interested in more
  strict checks can use `-Xclang -fno-skip-odr-check-in-gmf` to get the
  existing behavior.
---
 clang/docs/ReleaseNotes.rst   |  3 +-
 clang/docs/StandardCPlusPlusModules.rst   | 22 
 clang/include/clang/Basic/LangOptions.def |  1 +
 clang/include/clang/Driver/Options.td |  8 +++
 clang/include/clang/Serialization/ASTReader.h |  6 +-
 clang/lib/Driver/ToolChains/Clang.cpp |  8 +++
 clang/lib/Serialization/ASTReader.cpp |  2 +-
 clang/lib/Serialization/ASTReaderDecl.cpp | 17 +++---
 clang/lib/Serialization/ASTWriter.cpp |  2 +-
 clang/lib/Serialization/ASTWriterDecl.cpp |  8 +--
 .../Driver/modules-skip-odr-check-in-gmf.cpp  | 10 
 clang/test/Modules/concept.cppm   | 23 +---
 clang/test/Modules/polluted-operator.cppm | 18 +-
 clang/test/Modules/pr76638.cppm   | 16 +-
 clang/test/Modules/skip-odr-check-in-gmf.cppm | 56 +++
 15 files changed, 170 insertions(+), 30 deletions(-)
 create mode 100644 clang/test/Driver/modules-skip-odr-check-in-gmf.cpp
 create mode 100644 clang/test/Modules/skip-odr-check-in-gmf.cppm

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index bac6c7162a45b..74f34eeef65f0 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -64,7 +64,8 @@ C++20 Feature Support
 
 - Clang won't perform ODR checks for decls in the global module fragment any
   more to ease the implementation and improve the user's using experience.
-  This follows the MSVC's behavior.
+  This follows the MSVC's behavior. Users interested in testing the more strict
+  behavior can use the flag '-Xclang -fno-skip-odr-check-in-gmf'.
   (`#79240 `_).
 
 C++23 Feature Support
diff --git a/clang/docs/StandardCPlusPlusModules.rst 
b/clang/docs/StandardCPlusPlusModules.rst
index 4e853990a7338..44870f210cde6 100644
--- a/clang/docs/StandardCPlusPlusModules.rst
+++ b/clang/docs/StandardCPlusPlusModules.rst
@@ -457,6 +457,28 @@ Note that **currently** the compiler doesn't consider 
inconsistent macro definit
 Currently Clang would accept the above example. But it may produce surprising 
results if the
 debugging code depends on consistent use of ``NDEBUG`` also in other 
translation units.
 
+Definitions consistency
+^^^
+
+The C++ language defines that same declarations in different translation units 
should have
+the same definition, as known as ODR (One Definition Rule). Prior to modules, 
the translation
+units don't dependent on each other and the compiler itself don't and can't 
perform a strong
+ODR violation check. Sometimes it is the linker does some jobs related to ODR, 
where the
+higher level semantics are missing. With the introduction of modules, now the 
compiler have
+the chance to perform ODR violations with language semantics across 
translation units.
+
+However, in the practice we found the existing ODR checking mechanism may be 
too aggressive.
+In the many issue reports about ODR violation diagnostics, most of them are 
false positive
+ODR violations and the true positive ODR violations are rarely reported. Also 
MSVC don't
+perform ODR check for declarations in the global module fragment.
+
+So in order to get better user experience, save the time checking ODR and keep 
consistent
+behavior with MSVC, we disabled the ODR check for the declarations in the 
global module
+fragment by default. Users who want more strict check can still use the
+``-Xclang -fno-skip-odr-check-in-gmf`` flag 

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 created 
https://github.com/llvm/llvm-project/pull/79959

Close https://github.com/llvm/llvm-project/issues/79240

Cite the comment from @mizvekov in
//github.com/llvm/llvm-project/issues/79240:

> There are two kinds of bugs / issues relevant here:
>
> Clang bugs that this change hides
> Here we can add a Frontend flag that disables the GMF ODR check, just
> so
> we can keep tracking, testing and fixing these issues.
> The Driver would just always pass that flag.
> We could add that flag in this current issue.
> Bugs in user code:
> I don't think it's worth adding a corresponding Driver flag for
> controlling the above Frontend flag, since we intend it's behavior to
> become default as we fix the problems, and users interested in testing
> the more strict behavior can just use the Frontend flag directly.

This patch follows the suggestion:
- Introduce the CC1 flag `-fskip-odr-check-in-gmf` which is by default off, so 
that the every existing test will still be tested with checking ODR violations.
- Passing `-fskip-odr-check-in-gmf` in the driver to keep the behavior we 
intended.
- Edit the document to tell the users who are still interested in more strict 
checks can use `-Xclang -fno-skip-odr-check-in-gmf` to get the existing 
behavior.

>From 046ec7d3e8f509d830e2e6081d697415859811c2 Mon Sep 17 00:00:00 2001
From: Chuanqi Xu 
Date: Tue, 30 Jan 2024 15:57:35 +0800
Subject: [PATCH] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf

Close https://github.com/llvm/llvm-project/issues/79240

Cite the comment from @mizvekov in
//github.com/llvm/llvm-project/issues/79240:

> There are two kinds of bugs / issues relevant here:
>
> Clang bugs that this change hides
> Here we can add a Frontend flag that disables the GMF ODR check, just
> so
> we can keep tracking, testing and fixing these issues.
> The Driver would just always pass that flag.
> We could add that flag in this current issue.
> Bugs in user code:
> I don't think it's worth adding a corresponding Driver flag for
> controlling the above Frontend flag, since we intend it's behavior to
> become default as we fix the problems, and users interested in testing
> the more strict behavior can just use the Frontend flag directly.

This patch follows the suggestion:
- Introduce the CC1 flag `-fskip-odr-check-in-gmf` which is by default
  off,
  so that the every existing test will still be tested with checking ODR
  violations.
- Passing `-fskip-odr-check-in-gmf` in the driver to keep the behavior
  we intended.
- Edit the document to tell the users who are still interested in more
  strict checks can use `-Xclang -fno-skip-odr-check-in-gmf` to get the
  existing behavior.
---
 clang/docs/ReleaseNotes.rst   |  3 +-
 clang/docs/StandardCPlusPlusModules.rst   | 22 
 clang/include/clang/Basic/LangOptions.def |  1 +
 clang/include/clang/Driver/Options.td |  8 +++
 clang/include/clang/Serialization/ASTReader.h |  5 +-
 clang/lib/Driver/ToolChains/Clang.cpp |  8 +++
 clang/lib/Serialization/ASTReader.cpp |  2 +-
 clang/lib/Serialization/ASTReaderDecl.cpp | 14 ++---
 clang/lib/Serialization/ASTWriter.cpp |  2 +-
 clang/lib/Serialization/ASTWriterDecl.cpp |  8 +--
 .../Driver/modules-skip-odr-check-in-gmf.cpp  | 10 
 clang/test/Modules/concept.cppm   | 23 +---
 clang/test/Modules/polluted-operator.cppm | 18 +-
 clang/test/Modules/pr76638.cppm   | 16 +-
 clang/test/Modules/skip-odr-check-in-gmf.cppm | 56 +++
 15 files changed, 167 insertions(+), 29 deletions(-)
 create mode 100644 clang/test/Driver/modules-skip-odr-check-in-gmf.cpp
 create mode 100644 clang/test/Modules/skip-odr-check-in-gmf.cppm

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index bac6c7162a45b..44344ba330c5d 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -64,7 +64,8 @@ C++20 Feature Support
 
 - Clang won't perform ODR checks for decls in the global module fragment any
   more to ease the implementation and improve the user's using experience.
-  This follows the MSVC's behavior.
+  This follows the MSVC's behavior. Users interested in testing the more strict
+  behavior use the flag '-Xclang -fno-skip-odr-check-in-gmf'.
   (`#79240 `_).
 
 C++23 Feature Support
diff --git a/clang/docs/StandardCPlusPlusModules.rst 
b/clang/docs/StandardCPlusPlusModules.rst
index 4e853990a7338..c3d0c702a44c9 100644
--- a/clang/docs/StandardCPlusPlusModules.rst
+++ b/clang/docs/StandardCPlusPlusModules.rst
@@ -457,6 +457,28 @@ Note that **currently** the compiler doesn't consider 
inconsistent macro definit
 Currently Clang would accept the above example. But it may produce surprising 
results if the
 debugging code depends on consistent use of ``NDEBUG`` also in other 
translation units.
 
+Definitions consistency
+^^^
+
+The C++ 

[clang] [Serialization] Check for stack exhaustion when reading declarations (PR #79875)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

Another idea is to limit (or check) the threshold for 
`ASTReader::NumCurrentElementsDeserializing`. Then we still can print the stack 
trace by some utils in LLVM also we can get better performance than this. 

https://github.com/llvm/llvm-project/pull/79875
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Check for stack exhaustion when reading declarations (PR #79875)

[Chuanqi Xu via cfe-commits]

@@ -0,0 +1,43 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/usings.cppm -o 
%t/usings.pcm
+// RUN: %clang_cc1 -std=c++20 -fmodule-file=usings=%t/usings.pcm %t/use.cpp 
-verify -fsyntax-only -Wno-stack-exhausted
+
+// expected-no-diagnostics
+
+//--- usings.cppm
+export module usings;
+
+#define TYPES1(NAME) DECLARE(NAME##a) DECLARE(NAME##b) DECLARE(NAME##c) \
+DECLARE(NAME##d) DECLARE(NAME##e) DECLARE(NAME##f) DECLARE(NAME##g) \
+DECLARE(NAME##h) DECLARE(NAME##i) DECLARE(NAME##j) 
+#define TYPES2(NAME) TYPES1(NAME##a) TYPES1(NAME##b) TYPES1(NAME##c) \
+TYPES1(NAME##d) TYPES1(NAME##e) TYPES1(NAME##f) TYPES1(NAME##g) \
+TYPES1(NAME##h) TYPES1(NAME##i) TYPES1(NAME##j) 
+#define TYPES3(NAME) TYPES2(NAME##a) TYPES2(NAME##b) TYPES2(NAME##c) \
+TYPES2(NAME##d) TYPES2(NAME##e) TYPES2(NAME##f) TYPES2(NAME##g) \
+TYPES2(NAME##h) TYPES2(NAME##i) TYPES2(NAME##j) 
+#define TYPES4(NAME) TYPES3(NAME##a) TYPES3(NAME##b) TYPES3(NAME##c) \
+TYPES3(NAME##d) TYPES3(NAME##e) TYPES3(NAME##f) TYPES3(NAME##g)
+
+#define DECLARE(NAME) struct NAME {};
+TYPES4(Type)

ChuanqiXu9 wrote:

> I am not sure if there is an an approach that does not involve structural 
> changes to the AST (i.e. rewriting the ASTReaderDecl somehow, but keeping the 
> AST the same).

To my knowledge, it is only possible by introducing coroutines if we don't want 
to refactor the current deserializer significantly.

https://github.com/llvm/llvm-project/pull/79875
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Check for stack exhaustion when reading declarations (PR #79875)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 commented:

To make this testable, maybe we can refactor 
`clang::runWithSufficientStackSpace`  a little bit to make `DesiredStackSize` 
and `isStackNearlyExhausted::SufficientStack` configurable.

https://github.com/llvm/llvm-project/pull/79875
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Check for stack exhaustion when reading declarations (PR #79875)

[Chuanqi Xu via cfe-commits]

@@ -4099,7 +4099,9 @@ Decl *ASTReader::ReadDeclRecord(DeclID ID) {
   // calls to Decl::getASTContext() by Decl's methods will find the
   // TranslationUnitDecl without crashing.
   D->setDeclContext(Context.getTranslationUnitDecl());
-  Reader.Visit(D);
+
+  // Reading some declarations can result in deep recursion.
+  SemaObj->runWithSufficientStackSpace(DeclLoc, [&] { Reader.Visit(D); });

ChuanqiXu9 wrote:

Sema may not meaningful in ASTReader (e.g., we're compiling a pcm file). 
```suggestion
clang::runWithSufficientStackSpace(...)
```

Also in this way, we can get better diagnotsics.

https://github.com/llvm/llvm-project/pull/79875
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Check for stack exhaustion when reading declarations (PR #79875)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 edited 
https://github.com/llvm/llvm-project/pull/79875
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Load Specializations Lazily (PR #76774)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> > > As far as I can tell from [#76774 
> > > (comment)](https://github.com/llvm/llvm-project/pull/76774#issuecomment-1914177330)
> > >  above, the last push only changed the default value of 
> > > `LoadExternalSpecializationsLazily`. In that case, my test results from 
> > > [#76774 
> > > (comment)](https://github.com/llvm/llvm-project/pull/76774#issuecomment-1912182171)
> > >  remain fully valid, especially
> > > > Switching the new `LoadExternalSpecializationsLazily` to disabled by 
> > > > default (somehow the argument didn't work on its own) instead crashes, 
> > > > most of the cases involving `MultiOnDiskHashTable`. I suspect some kind 
> > > > of memory error maybe?
> > 
> > 
> > No, this newest patch changes besides changing the default value. For 
> > example, 
> > [22c9d11#diff-125f472e690aa3d973bc42aa3c5d580226c5c47661551aca2889f960681aa64dR232-R237](https://github.com/llvm/llvm-project/commit/22c9d1145eb57d9c2cb2ef490b7c474598dd5d12#diff-125f472e690aa3d973bc42aa3c5d580226c5c47661551aca2889f960681aa64dR232-R237)
> >  now we won't write specializations into the hash table, while the original 
> > patch tries to load all the decls from the consumers' side only. This patch 
> > touches the producer's side.
> > Maybe I shouldn't push force to make the history clear..
> 
> If you want I can set you up with the ROOT project so that you can debug the 
> failures easier.

Thanks. Although I don't understand how can it be, it will be better if things 
can be easier.

https://github.com/llvm/llvm-project/pull/76774
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 commented:

Thanks for looking into this. I haven't looked it in details. Given this is 
complex, it should take a relative longer time.

Here is some quick feedbacks:
- Every time we change the non-static data members of AST nodes, we need to 
update the serializations. Otherwise, modules won't work.
- Every time we change the LLVM intrinsics, we need to change the documentation.
- Every time we change the LLVM with a functional change, we need a test in the 
LLVM part.

And there is something I haven't fully understand:
- I feel odd about `helperFunction`. I don't understand the necessity and I 
feel we should make it in the middle end.
- I feel odd about `IsSuspendNoThrow`. Can't we make it simply during the 
CodeGen time by the attribute of `await_suspend`?.
- I am wondering if we can get rid of the introduction of `OpaqueFramePtr` from 
`CoroutineSuspendExpr` either in CodeGen part or in Sema part.

But I am not sure if my feelings are correct. I need more time looking into it.

https://github.com/llvm/llvm-project/pull/79712
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Load Specializations Lazily (PR #76774)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> As far as I can tell from [#76774 
> (comment)](https://github.com/llvm/llvm-project/pull/76774#issuecomment-1914177330)
>  above, the last push only changed the default value of 
> `LoadExternalSpecializationsLazily`. In that case, my test results from 
> [#76774 
> (comment)](https://github.com/llvm/llvm-project/pull/76774#issuecomment-1912182171)
>  remain fully valid, especially
> 
> > Switching the new `LoadExternalSpecializationsLazily` to disabled by 
> > default (somehow the argument didn't work on its own) instead crashes, most 
> > of the cases involving `MultiOnDiskHashTable`. I suspect some kind of 
> > memory error maybe?

No, this newest patch changes  besides changing the default value. For example, 
https://github.com/llvm/llvm-project/pull/76774/commits/22c9d1145eb57d9c2cb2ef490b7c474598dd5d12#diff-125f472e690aa3d973bc42aa3c5d580226c5c47661551aca2889f960681aa64dR232-R237
 now we won't write specializations into the hash table, while the original 
patch tries to load all the decls from the consumers' side only. This patch 
touches the producer's side.

Maybe I shouldn't push force to make the history clear..

https://github.com/llvm/llvm-project/pull/76774
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Load Specializations Lazily (PR #76774)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> The newest version of this patch still doesn't work correctly. Switching the 
> new `LoadExternalSpecializationsLazily` to disabled by default (somehow the 
> argument didn't work on its own) instead crashes, most of the cases involving 
> `MultiOnDiskHashTable`. I suspect some kind of memory error maybe? Sorry to 
> not be more helpful at the moment...

Thanks for testing it. It shows it is really complex. Maybe I am missing some 
points.

> How about taking a step at a time with this patch. Perhaps we should 
> introduce the on-disk hash table infrastructure and always deserialize 
> everything. Then we can verify that part works on our build infrastructure 
> and then move on with the deferring the template loading. I believe that 
> should be relatively easy to achieve with the current version of the patch.
> 
> Essentially I am proposing making `-fno-load-external-specializations-lazily` 
> default so that we can test it without having to modify build 
> infrastructure...

Good suggestion. It sounds good to me. Then we can test it and improve it step 
by step. I've made it. Now anything in the patch shouldn't be accessed without 
`-fload-external-specializations-lazily`.

https://github.com/llvm/llvm-project/pull/76774
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 4118082 - [C++20] [Modules] Remove previous workaround for odr hashing enums

[Chuanqi Xu via cfe-commits]

Author: Chuanqi Xu
Date: 2024-01-29T14:35:23+08:00
New Revision: 4118082f651a05cca258c684ab1199578b57afac

URL: 
https://github.com/llvm/llvm-project/commit/4118082f651a05cca258c684ab1199578b57afac
DIFF: 
https://github.com/llvm/llvm-project/commit/4118082f651a05cca258c684ab1199578b57afac.diff

LOG: [C++20] [Modules] Remove previous workaround for odr hashing enums

Previosly we land
https://github.com/llvm/llvm-project/commit/085eae6b863881fb9fda323e5b672b04a00ed19e
to workaround the false positive ODR violations in
https://github.com/llvm/llvm-project/issues/76638.

However, we decided to not perform ODR checks for decls from GMF in
https://github.com/llvm/llvm-project/issues/79240 and we land the
corresponding change. So we should be able to remove the workaround now.

The original tests get remained.

Added: 
clang/test/Modules/cxx20-modules-enum-odr.cppm

Modified: 
clang/lib/AST/ODRHash.cpp

Removed: 




diff  --git a/clang/lib/AST/ODRHash.cpp b/clang/lib/AST/ODRHash.cpp
index 5b98646a1e8dc0d..2dbc259138a897d 100644
--- a/clang/lib/AST/ODRHash.cpp
+++ b/clang/lib/AST/ODRHash.cpp
@@ -745,55 +745,8 @@ void ODRHash::AddEnumDecl(const EnumDecl *Enum) {
   if (Enum->isScoped())
 AddBoolean(Enum->isScopedUsingClassTag());
 
-  if (Enum->getIntegerTypeSourceInfo()) {
-// FIMXE: This allows two enums with 
diff erent spellings to have the same
-// hash.
-//
-//  // mod1.cppm
-//  module;
-//  extern "C" {
-//  typedef unsigned __int64 size_t;
-//  }
-//  namespace std {
-//  using :: size_t;
-//  }
-//
-//  extern "C++" {
-//  namespace std {
-//  enum class align_val_t : std::size_t {};
-//  }
-//  }
-//
-//  export module mod1;
-//  export using std::align_val_t;
-//
-//  // mod2.cppm
-//  module;
-//  extern "C" {
-//  typedef unsigned __int64 size_t;
-//  }
-//
-//  extern "C++" {
-//  namespace std {
-//  enum class align_val_t : size_t {};
-//  }
-//  }
-//
-//  export module mod2;
-//  import mod1;
-//  export using std::align_val_t;
-//
-// The above example should be disallowed since it violates
-// [basic.def.odr]p14:
-//
-//Each such definition shall consist of the same sequence of tokens
-//
-// The definitions of `std::align_val_t` in two module units have 
diff erent
-// spellings but we failed to give an error here.
-//
-// See https://github.com/llvm/llvm-project/issues/76638 for details.
+  if (Enum->getIntegerTypeSourceInfo())
 AddQualType(Enum->getIntegerType().getCanonicalType());
-  }
 
   // Filter out sub-Decls which will not be processed in order to get an
   // accurate count of Decl's.

diff  --git a/clang/test/Modules/cxx20-modules-enum-odr.cppm 
b/clang/test/Modules/cxx20-modules-enum-odr.cppm
new file mode 100644
index 000..831c01143a27ba5
--- /dev/null
+++ b/clang/test/Modules/cxx20-modules-enum-odr.cppm
@@ -0,0 +1,51 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/mod1.cppm -emit-module-interface -o 
%t/mod1.pcm
+// RUN: %clang_cc1 -std=c++20 %t/mod2.cppm -emit-module-interface -o 
%t/mod2.pcm
+// RUN: %clang_cc1 -std=c++20 %t/test.cpp -fprebuilt-module-path=%t -verify 
-fsyntax-only
+
+//--- size_t.h
+
+extern "C" {
+typedef unsigned int size_t;
+}
+
+//--- csize_t
+namespace std {
+using :: size_t;
+}
+
+//--- align.h
+namespace std {
+enum class align_val_t : size_t {};
+}
+
+//--- mod1.cppm
+module;
+#include "size_t.h"
+#include "align.h"
+export module mod1;
+namespace std {
+export using std::align_val_t;
+}
+
+//--- mod2.cppm
+module;
+#include "size_t.h"
+#include "csize_t"
+#include "align.h"
+export module mod2;
+namespace std {
+export using std::align_val_t;
+}
+
+//--- test.cpp
+// expected-no-diagnostics
+import mod1;
+import mod2;
+void test() {
+std::align_val_t v;
+}
+



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] a0b6747 - [C++20] [Modules] Don't perform ODR checks in GMF

[Chuanqi Xu via cfe-commits]

Author: Chuanqi Xu
Date: 2024-01-29T11:44:59+08:00
New Revision: a0b6747804e46665ecfd00295b60432bfe1775b6

URL: 
https://github.com/llvm/llvm-project/commit/a0b6747804e46665ecfd00295b60432bfe1775b6
DIFF: 
https://github.com/llvm/llvm-project/commit/a0b6747804e46665ecfd00295b60432bfe1775b6.diff

LOG: [C++20] [Modules] Don't perform ODR checks in GMF

Close https://github.com/llvm/llvm-project/issues/79240.

See the linked issue for details. Given the frequency of issue reporting
about false positive ODR checks (I received private issue reports too),
I'd like to backport this to 18.x too.

Added: 


Modified: 
clang/docs/ReleaseNotes.rst
clang/include/clang/Serialization/ASTReader.h
clang/lib/Serialization/ASTReader.cpp
clang/lib/Serialization/ASTReaderDecl.cpp
clang/lib/Serialization/ASTWriter.cpp
clang/lib/Serialization/ASTWriterDecl.cpp
clang/test/Modules/concept.cppm
clang/test/Modules/no-eager-load.cppm
clang/test/Modules/polluted-operator.cppm
clang/test/Modules/pr76638.cppm

Removed: 




diff  --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 2aa8fea4ad67c0..9d68be469dac39 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -62,6 +62,11 @@ C++ Language Changes
 C++20 Feature Support
 ^
 
+- Clang won't perform ODR checks for decls in the global module fragment any
+  more to ease the implementation and improve the user's using experience.
+  This follows the MSVC's behavior.
+  (`#79240 `_).
+
 C++23 Feature Support
 ^
 

diff  --git a/clang/include/clang/Serialization/ASTReader.h 
b/clang/include/clang/Serialization/ASTReader.h
index dd1451bbf2d2c9..ba06ab0cd45097 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -2452,6 +2452,10 @@ class BitsUnpacker {
   uint32_t CurrentBitsIndex = ~0;
 };
 
+inline bool isFromExplicitGMF(const Decl *D) {
+  return D->getOwningModule() && 
D->getOwningModule()->isExplicitGlobalModule();
+}
+
 } // namespace clang
 
 #endif // LLVM_CLANG_SERIALIZATION_ASTREADER_H

diff  --git a/clang/lib/Serialization/ASTReader.cpp 
b/clang/lib/Serialization/ASTReader.cpp
index 89b044a61a7b15..2abe5e44e2e988 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -9747,6 +9747,9 @@ void ASTReader::finishPendingActions() {
 
 if (!FD->isLateTemplateParsed() &&
 !NonConstDefn->isLateTemplateParsed() &&
+// We only perform ODR checks for decls not in the explicit
+// global module fragment.
+!isFromExplicitGMF(FD) &&
 FD->getODRHash() != NonConstDefn->getODRHash()) {
   if (!isa(FD)) {
 PendingFunctionOdrMergeFailures[FD].push_back(NonConstDefn);

diff  --git a/clang/lib/Serialization/ASTReaderDecl.cpp 
b/clang/lib/Serialization/ASTReaderDecl.cpp
index 867f4c47eaeceb..29ece2a132bf8c 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -804,8 +804,10 @@ void ASTDeclReader::VisitEnumDecl(EnumDecl *ED) {
   ED->setScopedUsingClassTag(EnumDeclBits.getNextBit());
   ED->setFixed(EnumDeclBits.getNextBit());
 
-  ED->setHasODRHash(true);
-  ED->ODRHash = Record.readInt();
+  if (!isFromExplicitGMF(ED)) {
+ED->setHasODRHash(true);
+ED->ODRHash = Record.readInt();
+  }
 
   // If this is a definition subject to the ODR, and we already have a
   // definition, merge this one into it.
@@ -827,7 +829,9 @@ void ASTDeclReader::VisitEnumDecl(EnumDecl *ED) {
   Reader.MergedDeclContexts.insert(std::make_pair(ED, OldDef));
   ED->demoteThisDefinitionToDeclaration();
   Reader.mergeDefinitionVisibility(OldDef, ED);
-  if (OldDef->getODRHash() != ED->getODRHash())
+  // We don't want to check the ODR hash value for declarations from global
+  // module fragment.
+  if (!isFromExplicitGMF(ED) && OldDef->getODRHash() != ED->getODRHash())
 Reader.PendingEnumOdrMergeFailures[OldDef].push_back(ED);
 } else {
   OldDef = ED;
@@ -866,6 +870,9 @@ ASTDeclReader::VisitRecordDeclImpl(RecordDecl *RD) {
 
 void ASTDeclReader::VisitRecordDecl(RecordDecl *RD) {
   VisitRecordDeclImpl(RD);
+  // We should only reach here if we're in C/Objective-C. There is no
+  // global module fragment.
+  assert(!isFromExplicitGMF(RD));
   RD->setODRHash(Record.readInt());
 
   // Maintain the invariant of a redeclaration chain containing only
@@ -1094,8 +1101,10 @@ void ASTDeclReader::VisitFunctionDecl(FunctionDecl *FD) {
   if (FD->isExplicitlyDefaulted())
 FD->setDefaultLoc(readSourceLocation());
 
-  FD->ODRHash = Record.readInt();
-  FD->setHasODRHash(true);
+  if (!isFromExplicitGMF(FD)) {
+FD->ODRHash = Record.readInt();
+FD->setHasODRHash(true);
+  }
 
   

[clang] [clang-scan-deps] Fix check for empty `Compilation` (PR #75545)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 edited 
https://github.com/llvm/llvm-project/pull/75545
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-scan-deps] Fix check for empty `Compilation` (PR #75545)

[Chuanqi Xu via cfe-commits]

@@ -728,7 +728,7 @@ getCompilationDataBase(int argc, char **argv, std::string 
) {
*Diags);
   std::unique_ptr C(
   TheDriver.BuildCompilation(CommandLine));
-  if (!C)
+  if (C->getJobs().empty())

ChuanqiXu9 wrote:

```suggestion
  if (!C || C->getJobs().empty())
```

https://github.com/llvm/llvm-project/pull/75545
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-scan-deps] Fix check for empty `Compilation` (PR #75545)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 commented:

It will be better to have a test for this.

https://github.com/llvm/llvm-project/pull/75545
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [C++20] [Modules] [Itanium ABI] Generate the vtable in the module unit of dynamic classes (PR #75912)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 updated 
https://github.com/llvm/llvm-project/pull/75912

>From 7a5c4ccd37b263a4d3d01df16591b576a64e839f Mon Sep 17 00:00:00 2001
From: Chuanqi Xu 
Date: Tue, 19 Dec 2023 17:00:59 +0800
Subject: [PATCH] [C++20] [Modules] [Itanium ABI] Generate the vtable in the
 module unit of dynamic classes

Close https://github.com/llvm/llvm-project/issues/70585 and reflect
https://github.com/itanium-cxx-abi/cxx-abi/issues/170.

The significant change of the patch is: for dynamic classes attached to
module units, we generate the vtable to the attached module units
directly and the key functions for such classes is meaningless.
---
 clang/lib/CodeGen/CGVTables.cpp   | 28 ++
 clang/lib/CodeGen/CodeGenModule.cpp   |  9 +
 clang/lib/CodeGen/ItaniumCXXABI.cpp   |  6 +++
 clang/lib/Sema/SemaDecl.cpp   |  9 +
 clang/lib/Sema/SemaDeclCXX.cpp| 13 +--
 clang/lib/Serialization/ASTReaderDecl.cpp | 13 ++-
 clang/lib/Serialization/ASTWriterDecl.cpp | 13 +--
 clang/test/CodeGenCXX/modules-vtable.cppm | 27 -
 clang/test/CodeGenCXX/pr70585.cppm| 47 +++
 9 files changed, 139 insertions(+), 26 deletions(-)
 create mode 100644 clang/test/CodeGenCXX/pr70585.cppm

diff --git a/clang/lib/CodeGen/CGVTables.cpp b/clang/lib/CodeGen/CGVTables.cpp
index 27a2cab4f75319..5c8499d2594a1d 100644
--- a/clang/lib/CodeGen/CGVTables.cpp
+++ b/clang/lib/CodeGen/CGVTables.cpp
@@ -1046,6 +1046,11 @@ CodeGenModule::getVTableLinkage(const CXXRecordDecl *RD) 
{
   if (!RD->isExternallyVisible())
 return llvm::GlobalVariable::InternalLinkage;
 
+  // V-tables for non-template classes with an owning module are always
+  // uniquely emitted in that module.
+  if (Module *M = RD->getOwningModule(); M && M->isNamedModule())
+return llvm::GlobalVariable::ExternalLinkage;
+
   // We're at the end of the translation unit, so the current key
   // function is fully correct.
   const CXXMethodDecl *keyFunction = Context.getCurrentKeyFunction(RD);
@@ -1180,6 +1185,21 @@ bool CodeGenVTables::isVTableExternal(const 
CXXRecordDecl *RD) {
   TSK == TSK_ExplicitInstantiationDefinition)
 return false;
 
+  // Itanium C++ ABI [5.2.3]:
+  // Virtual tables for dynamic classes are emitted as follows:
+  //
+  // - If the class is templated, the tables are emitted in every object that
+  // references any of them.
+  // - Otherwise, if the class is attached to a module, the tables are uniquely
+  // emitted in the object for the module unit in which it is defined.
+  // - Otherwise, if the class has a key function (see below), the tables are
+  // emitted in the object for the translation unit containing the definition 
of
+  // the key function. This is unique if the key function is not inline.
+  // - Otherwise, the tables are emitted in every object that references any of
+  // them.
+  if (Module *M = RD->getOwningModule(); M && M->isNamedModule())
+return M != CGM.getContext().getCurrentNamedModule();
+
   // Otherwise, if the class doesn't have a key function (possibly
   // anymore), the vtable must be defined here.
   const CXXMethodDecl *keyFunction = 
CGM.getContext().getCurrentKeyFunction(RD);
@@ -1189,13 +1209,7 @@ bool CodeGenVTables::isVTableExternal(const 
CXXRecordDecl *RD) {
   const FunctionDecl *Def;
   // Otherwise, if we don't have a definition of the key function, the
   // vtable must be defined somewhere else.
-  if (!keyFunction->hasBody(Def))
-return true;
-
-  assert(Def && "The body of the key function is not assigned to Def?");
-  // If the non-inline key function comes from another module unit, the vtable
-  // must be defined there.
-  return Def->isInAnotherModuleUnit() && !Def->isInlineSpecified();
+  return !keyFunction->hasBody(Def);
 }
 
 /// Given that we're currently at the end of the translation unit, and
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp 
b/clang/lib/CodeGen/CodeGenModule.cpp
index d78f2594a23764..4863f7348ed99a 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -6738,6 +6738,15 @@ void CodeGenModule::EmitTopLevelDecl(Decl *D) {
 if (ES->hasExternalDefinitions(D) == ExternalASTSource::EK_Never)
   DI->completeUnusedClass(*CRD);
 }
+// If we're emitting a dynamic class from the importable module we're
+// emitting, we always need to emit the virtual table according to the ABI
+// requirement.
+if (CRD->getOwningModule() &&
+CRD->getOwningModule()->isInterfaceOrPartition() &&
+CRD->getDefinition() && CRD->isDynamicClass() &&
+CRD->getOwningModule() == getContext().getCurrentNamedModule())
+  EmitVTable(CRD);
+
 // Emit any static data members, they may be definitions.
 for (auto *I : CRD->decls())
   if (isa(I) || isa(I))
diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp 
b/clang/lib/CodeGen/ItaniumCXXABI.cpp
index d173806ec8ce53..1acf8b3e0659ff 

[clang] [C++20] [Modules] [Itanium ABI] Generate the vtable in the module unit of dynamic classes (PR #75912)

[Chuanqi Xu via cfe-commits]

@@ -1046,6 +1046,15 @@ CodeGenModule::getVTableLinkage(const CXXRecordDecl *RD) 
{
   if (!RD->isExternallyVisible())
 return llvm::GlobalVariable::InternalLinkage;
 
+  // Previously we'll decide the linkage of the vtable by the linkage
+  // of the key function. But within modules, the concept of key functions
+  // becomes meaningless. So the linkage of the vtable should always be
+  // external if the class is externally visible.
+  //
+  // TODO: How about the case of AppleKext, DLLExportAttr and DLLImportAttr.

ChuanqiXu9 wrote:

I've updated the comments. And I'd like to leave the simplification to later 
patches.

https://github.com/llvm/llvm-project/pull/75912
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [C++20] [Modules] [Itanium ABI] Generate the vtable in the module unit of dynamic classes (PR #75912)

[Chuanqi Xu via cfe-commits]

@@ -1801,6 +1801,12 @@ void ItaniumCXXABI::emitVTableDefinitions(CodeGenVTables 
,
   if (VTable->hasInitializer())
 return;
 
+  // If the class are attached to a C++ named module other than the one

ChuanqiXu9 wrote:

Done

https://github.com/llvm/llvm-project/pull/75912
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [C++20] [Modules] [Itanium ABI] Generate the vtable in the module unit of dynamic classes (PR #75912)

[Chuanqi Xu via cfe-commits]

@@ -0,0 +1,50 @@
+// REQUIRES: !system-windows
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/layer1.cppm -triple %itanium_abi_triple \
+// RUN: -emit-module-interface -o %t/foo-layer1.pcm
+// RUN: %clang_cc1 -std=c++20 %t/layer2.cppm -triple %itanium_abi_triple  \
+// RUN: -emit-module-interface -fmodule-file=foo:layer1=%t/foo-layer1.pcm \
+// RUN: -o %t/foo-layer2.pcm
+// RUN: %clang_cc1 -std=c++20 %t/foo-layer1.pcm -S -emit-llvm -o - | FileCheck 
%t/layer1.cppm
+// RUN: %clang_cc1 -std=c++20 %t/foo-layer2.pcm -S -emit-llvm -o - \
+// RUN: -fmodule-file=foo:layer1=%t/foo-layer1.pcm | FileCheck 
%t/layer2.cppm
+
+//--- layer1.cppm
+export module foo:layer1;
+struct Fruit {
+virtual ~Fruit() = default;
+virtual void eval() = 0;
+};
+struct Banana : public Fruit {
+Banana() {}
+void eval() override;
+};
+

ChuanqiXu9 wrote:

I've just tried to update it here. If you feel it is too sparse, I can try to 
make it into 2 patches.

https://github.com/llvm/llvm-project/pull/75912
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [C++20] [Modules] [Itanium ABI] Generate the vtable in the module unit of dynamic classes (PR #75912)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 updated 
https://github.com/llvm/llvm-project/pull/75912

>From 630e59738990c3dd570065b8b7a050d822d68df0 Mon Sep 17 00:00:00 2001
From: Chuanqi Xu 
Date: Tue, 19 Dec 2023 17:00:59 +0800
Subject: [PATCH] [C++20] [Modules] [Itanium ABI] Generate the vtable in the
 module unit of dynamic classes

Close https://github.com/llvm/llvm-project/issues/70585 and reflect
https://github.com/itanium-cxx-abi/cxx-abi/issues/170.

The significant change of the patch is: for dynamic classes attached to
module units, we generate the vtable to the attached module units
directly and the key functions for such classes is meaningless.
---
 clang/lib/CodeGen/CGVTables.cpp   | 28 ++
 clang/lib/CodeGen/CodeGenModule.cpp   |  7 
 clang/lib/CodeGen/ItaniumCXXABI.cpp   |  6 +++
 clang/lib/Sema/SemaDecl.cpp   |  9 +
 clang/lib/Sema/SemaDeclCXX.cpp| 13 +--
 clang/lib/Serialization/ASTReaderDecl.cpp | 15 +++-
 clang/lib/Serialization/ASTWriterDecl.cpp | 15 +---
 clang/test/CodeGenCXX/modules-vtable.cppm | 27 -
 clang/test/CodeGenCXX/pr70585.cppm| 47 +++
 9 files changed, 139 insertions(+), 28 deletions(-)
 create mode 100644 clang/test/CodeGenCXX/pr70585.cppm

diff --git a/clang/lib/CodeGen/CGVTables.cpp b/clang/lib/CodeGen/CGVTables.cpp
index 27a2cab4f75319..5c8499d2594a1d 100644
--- a/clang/lib/CodeGen/CGVTables.cpp
+++ b/clang/lib/CodeGen/CGVTables.cpp
@@ -1046,6 +1046,11 @@ CodeGenModule::getVTableLinkage(const CXXRecordDecl *RD) 
{
   if (!RD->isExternallyVisible())
 return llvm::GlobalVariable::InternalLinkage;
 
+  // V-tables for non-template classes with an owning module are always
+  // uniquely emitted in that module.
+  if (Module *M = RD->getOwningModule(); M && M->isNamedModule())
+return llvm::GlobalVariable::ExternalLinkage;
+
   // We're at the end of the translation unit, so the current key
   // function is fully correct.
   const CXXMethodDecl *keyFunction = Context.getCurrentKeyFunction(RD);
@@ -1180,6 +1185,21 @@ bool CodeGenVTables::isVTableExternal(const 
CXXRecordDecl *RD) {
   TSK == TSK_ExplicitInstantiationDefinition)
 return false;
 
+  // Itanium C++ ABI [5.2.3]:
+  // Virtual tables for dynamic classes are emitted as follows:
+  //
+  // - If the class is templated, the tables are emitted in every object that
+  // references any of them.
+  // - Otherwise, if the class is attached to a module, the tables are uniquely
+  // emitted in the object for the module unit in which it is defined.
+  // - Otherwise, if the class has a key function (see below), the tables are
+  // emitted in the object for the translation unit containing the definition 
of
+  // the key function. This is unique if the key function is not inline.
+  // - Otherwise, the tables are emitted in every object that references any of
+  // them.
+  if (Module *M = RD->getOwningModule(); M && M->isNamedModule())
+return M != CGM.getContext().getCurrentNamedModule();
+
   // Otherwise, if the class doesn't have a key function (possibly
   // anymore), the vtable must be defined here.
   const CXXMethodDecl *keyFunction = 
CGM.getContext().getCurrentKeyFunction(RD);
@@ -1189,13 +1209,7 @@ bool CodeGenVTables::isVTableExternal(const 
CXXRecordDecl *RD) {
   const FunctionDecl *Def;
   // Otherwise, if we don't have a definition of the key function, the
   // vtable must be defined somewhere else.
-  if (!keyFunction->hasBody(Def))
-return true;
-
-  assert(Def && "The body of the key function is not assigned to Def?");
-  // If the non-inline key function comes from another module unit, the vtable
-  // must be defined there.
-  return Def->isInAnotherModuleUnit() && !Def->isInlineSpecified();
+  return !keyFunction->hasBody(Def);
 }
 
 /// Given that we're currently at the end of the translation unit, and
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp 
b/clang/lib/CodeGen/CodeGenModule.cpp
index d78f2594a23764..b133695d90905b 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -6738,6 +6738,13 @@ void CodeGenModule::EmitTopLevelDecl(Decl *D) {
 if (ES->hasExternalDefinitions(D) == ExternalASTSource::EK_Never)
   DI->completeUnusedClass(*CRD);
 }
+// If we're emitting a dynamic class from the importable module we're 
emitting,
+// we always need to emit the virtual table according to the ABI 
requirement.
+if (CRD->getOwningModule() && 
CRD->getOwningModule()->isInterfaceOrPartition() &&
+CRD->getDefinition() && CRD->isDynamicClass() &&
+CRD->getOwningModule() == getContext().getCurrentNamedModule())
+  EmitVTable(CRD);
+
 // Emit any static data members, they may be definitions.
 for (auto *I : CRD->decls())
   if (isa(I) || isa(I))
diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp 
b/clang/lib/CodeGen/ItaniumCXXABI.cpp
index d173806ec8ce53..1acf8b3e0659ff 100644
--- 

[clang] Support C++20 Modules in clang-repl (PR #79261)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

It looks true but the test does the same thing as other tests to build modules

https://github.com/llvm/llvm-project/pull/79261
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Support C++20 Modules in clang-repl (PR #79261)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> For what it's worth, this test appears to fail when 
> `LLVM_DEFAULT_TARGET_TRIPLE` is changed (my script sets it to the output of 
> my Linux distribution's `clang -print-target-triple`)
> 
> ```
> error: PCH file was compiled for the target 'x86_64-pc-linux-gnu' but the 
> current translation unit is being compiled for target 
> 'x86_64-unknown-linux-gnu'
> error: module file 
> .../tools/clang/test/Interpreter/Output/cxx20-modules.cppm.tmp/mod.pcm cannot 
> be loaded due to a configuration mismatch with the current compilation 
> [-Wmodule-file-config-mismatch]
> error: Parsing failed.
> ```

Thanks. But I am not sure how should I change the test. Do you have any idea or 
suggestion? 

https://github.com/llvm/llvm-project/pull/79261
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Revert "[Modules] [HeaderSearch] Don't reenter headers if it is pragm… (PR #79396)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 approved this pull request.

LGTM. Sorry for forgetting testing lldb : (

https://github.com/llvm/llvm-project/pull/79396
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] bae1ada - [docs] [C++20] [Modules] Document how to import modules in clang-repl

[Chuanqi Xu via cfe-commits]

Author: Chuanqi Xu
Date: 2024-01-24T16:14:14+08:00
New Revision: bae1adae1c7cdf3b0bd618fc9cd5af251dc901ed

URL: 
https://github.com/llvm/llvm-project/commit/bae1adae1c7cdf3b0bd618fc9cd5af251dc901ed
DIFF: 
https://github.com/llvm/llvm-project/commit/bae1adae1c7cdf3b0bd618fc9cd5af251dc901ed.diff

LOG: [docs] [C++20] [Modules] Document how to import modules in clang-repl

We support to import C++20 named modules now in in clang-repl in
https://github.com/llvm/llvm-project/commit/dd3e6c87f3f4affd17d05a4d25fa77d224a98d94.

Then we should document how can we do that.

Added: 


Modified: 
clang/docs/StandardCPlusPlusModules.rst

Removed: 




diff  --git a/clang/docs/StandardCPlusPlusModules.rst 
b/clang/docs/StandardCPlusPlusModules.rst
index 81043ff25be02ea..4e853990a7338a1 100644
--- a/clang/docs/StandardCPlusPlusModules.rst
+++ b/clang/docs/StandardCPlusPlusModules.rst
@@ -1213,6 +1213,45 @@ instead a real binary. There are 4 potential solutions 
to the problem:
   $ clang-scan-deps -format=p1689 -- /clang++ 
-std=c++20 -resource-dir  mod.cppm -c -o mod.o
 
 
+Import modules with clang-repl
+==
+
+We're able to import C++20 named modules with clang-repl.
+
+Let's start with a simple example:
+
+.. code-block:: c++
+
+  // M.cppm
+  export module M;
+  export const char* Hello() {
+  return "Hello Interpreter for Modules!";
+  }
+
+We still need to compile the named module in ahead.
+
+.. code-block:: console
+
+  $ clang++ -std=c++20 M.cppm --precompile -o M.pcm
+  $ clang++ M.pcm -c -o M.o
+  $ clang++ -shared M.o -o libM.so
+
+Note that we need to compile the module unit into a dynamic library so that 
the clang-repl
+can load the object files of the module units.
+
+Then we are able to import module ``M`` in clang-repl.
+
+.. code-block:: console
+
+  $ clang-repl -Xcc=-std=c++20 -Xcc=-fprebuilt-module-path=.
+  # We need to load the dynamic library first before importing the modules.
+  clang-repl> %lib libM.so
+  clang-repl> import M;
+  clang-repl> extern "C" int printf(const char *, ...);
+  clang-repl> printf("%s\n", Hello());
+  Hello Interpreter for Modules!
+  clang-repl> %quit
+
 Possible Questions
 ==
 



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] c125965 - [NFC] Add more requirement to clang/test/Interpreter/cxx20-modules.cppm

[Chuanqi Xu via cfe-commits]

Author: Chuanqi Xu
Date: 2024-01-24T16:10:49+08:00
New Revision: c1259650e742e7b3053f6520729b4c1f44c27814

URL: 
https://github.com/llvm/llvm-project/commit/c1259650e742e7b3053f6520729b4c1f44c27814
DIFF: 
https://github.com/llvm/llvm-project/commit/c1259650e742e7b3053f6520729b4c1f44c27814.diff

LOG: [NFC] Add more requirement to clang/test/Interpreter/cxx20-modules.cppm

The previous test can't work on other platforms like PS4.

Address the comments in
https://github.com/llvm/llvm-project/pull/79261#issuecomment-1907589030

Added: 


Modified: 
clang/test/Interpreter/cxx20-modules.cppm

Removed: 




diff  --git a/clang/test/Interpreter/cxx20-modules.cppm 
b/clang/test/Interpreter/cxx20-modules.cppm
index bc2b722f6b5197e..2c6eba525519175 100644
--- a/clang/test/Interpreter/cxx20-modules.cppm
+++ b/clang/test/Interpreter/cxx20-modules.cppm
@@ -1,3 +1,4 @@
+// REQUIRES: host-supports-jit, x86_64-linux
 // UNSUPPORTED: system-aix
 //
 // RUN: rm -rf %t



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Support C++20 Modules in clang-repl (PR #79261)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> Hi @ChuanqiXu9, the test you added is failing on the PS4 bot because for the 
> PS4 target, it requires an external linker which is not present/available. 
> Can the test be rewritten to not require linking?
> 
> https://lab.llvm.org/buildbot/#/builders/139/builds/57928
> 
> ```
> clang: error: unable to execute command: Executable "orbis-ld" doesn't exist!
> clang: error: linker command failed with exit code 1 (use -v to see 
> invocation)
> ```

Oh, maybe I need to add more requires to the test.

https://github.com/llvm/llvm-project/pull/79261
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Support C++20 Modules in clang-repl (PR #79261)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 closed 
https://github.com/llvm/llvm-project/pull/79261
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Support C++20 Modules in clang-repl (PR #79261)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 created 
https://github.com/llvm/llvm-project/pull/79261

This comes from when I playing around clang-repl with moduels : )

I succeeded to import std with https://libcxx.llvm.org/Modules.html and calling 
`std::printf` after this patch.

I want to put the documentation part to 
https://clang.llvm.org/docs/StandardCPlusPlusModules.html in a separate commit.

>From 70cb9ac882605c3557dc41ea13a4b8945c59e9c2 Mon Sep 17 00:00:00 2001
From: Chuanqi Xu 
Date: Wed, 24 Jan 2024 14:21:25 +0800
Subject: [PATCH] Support C++20 Modules in clang-repl

---
 clang/docs/ReleaseNotes.rst   |  2 ++
 clang/include/clang/Basic/LangOptions.def |  2 +-
 clang/lib/Serialization/ASTReaderDecl.cpp |  6 ++---
 clang/test/Interpreter/cxx20-modules.cppm | 31 +++
 4 files changed, 37 insertions(+), 4 deletions(-)
 create mode 100644 clang/test/Interpreter/cxx20-modules.cppm

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index fc7511e9136734..db3d74e124e7d1 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -191,6 +191,8 @@ Crash and bug fixes
 Improvements
 
 
+- Support importing C++20 modules in clang-repl.
+
 Moved checkers
 ^^
 
diff --git a/clang/include/clang/Basic/LangOptions.def 
b/clang/include/clang/Basic/LangOptions.def
index 8fc75e1cca0399..1e671a7c460163 100644
--- a/clang/include/clang/Basic/LangOptions.def
+++ b/clang/include/clang/Basic/LangOptions.def
@@ -485,7 +485,7 @@ VALUE_LANGOPT(FuchsiaAPILevel, 32, 0, "Fuchsia API level")
 // on large _BitInts.
 BENIGN_VALUE_LANGOPT(MaxBitIntWidth, 32, 128, "Maximum width of a _BitInt")
 
-LANGOPT(IncrementalExtensions, 1, 0, " True if we want to process statements"
+COMPATIBLE_LANGOPT(IncrementalExtensions, 1, 0, " True if we want to process 
statements"
 "on the global scope, ignore EOF token and continue later on (thus "
 "avoid tearing the Lexer and etc. down). Controlled by "
 "-fincremental-extensions.")
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp 
b/clang/lib/Serialization/ASTReaderDecl.cpp
index a149d82153037f..867f4c47eaeceb 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -3286,10 +3286,10 @@ DeclContext 
*ASTDeclReader::getPrimaryContextForMerging(ASTReader ,
   if (auto *OID = dyn_cast(DC))
 return OID->getDefinition();
 
-  // We can see the TU here only if we have no Sema object. In that case,
-  // there's no TU scope to look in, so using the DC alone is sufficient.
+  // We can see the TU here only if we have no Sema object. It is possible
+  // we're in clang-repl so we still need to get the primary context.
   if (auto *TU = dyn_cast(DC))
-return TU;
+return TU->getPrimaryContext();
 
   return nullptr;
 }
diff --git a/clang/test/Interpreter/cxx20-modules.cppm 
b/clang/test/Interpreter/cxx20-modules.cppm
new file mode 100644
index 00..bc2b722f6b5197
--- /dev/null
+++ b/clang/test/Interpreter/cxx20-modules.cppm
@@ -0,0 +1,31 @@
+// UNSUPPORTED: system-aix
+//
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang -std=c++20 %t/mod.cppm --precompile \
+// RUN: -o %t/mod.pcm
+// RUN: %clang %t/mod.pcm -c -o %t/mod.o
+// RUN: %clang -shared %t/mod.o -o %t/libmod.so
+//
+// RUN: cat %t/import.cpp | env LD_LIBRARY_PATH=%t:$LD_LIBRARY_PATH \
+// RUN: clang-repl -Xcc=-std=c++20 -Xcc=-fmodule-file=M=%t/mod.pcm \
+// RUN: | FileCheck %t/import.cpp
+
+//--- mod.cppm
+export module M;
+export const char* Hello() {
+return "Hello Interpreter for Modules!";
+}
+
+//--- import.cpp
+
+%lib libmod.so
+
+import M;
+
+extern "C" int printf(const char *, ...);
+printf("%s\n", Hello());
+
+// CHECK: Hello Interpreter for Modules!

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Modules] [HeaderSearch] Don't reenter headers if it is pragma once (PR #76119)

[Chuanqi Xu via cfe-commits]

@@ -1458,7 +1458,7 @@ bool HeaderSearch::ShouldEnterIncludeFile(Preprocessor 
,
   } else {
 // Otherwise, if this is a #include of a file that was previously #import'd
 // or if this is the second #include of a #pragma once file, ignore it.
-if ((FileInfo.isPragmaOnce || FileInfo.isImport) && !TryEnterImported())

ChuanqiXu9 wrote:

I feel it will become harder to read. So I didn't inline it. But I move the 
definition to a closer place.

https://github.com/llvm/llvm-project/pull/76119
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Modules] [HeaderSearch] Don't reenter headers if it is pragma once (PR #76119)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 updated 
https://github.com/llvm/llvm-project/pull/76119

>From 65fdddbd501b36f7dcef2db3fd25a7a4a1f80a0b Mon Sep 17 00:00:00 2001
From: Chuanqi Xu 
Date: Thu, 21 Dec 2023 11:24:14 +0800
Subject: [PATCH] [Modules] [HeaderSearch] Don't reenter headers if it is
 pragma once of #import

Close https://github.com/llvm/llvm-project/issues/73023

The direct issue of https://github.com/llvm/llvm-project/issues/73023 is
that we entered a header which is marked as pragma once since the
compiler think it is OK if there is controlling macro.

It doesn't make sense. I feel like it should be sufficient to skip it
after we see the '#pragma once'.

>From the context, it looks like the workaround is primarily for
ObjectiveC. So we might need reviewers from OC.
---
 clang/lib/Lex/HeaderSearch.cpp | 78 +-
 clang/test/Modules/pr73023.cpp | 19 +
 2 files changed, 58 insertions(+), 39 deletions(-)
 create mode 100644 clang/test/Modules/pr73023.cpp

diff --git a/clang/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp
index 0f1090187734ff2..dfa974e9a67ede3 100644
--- a/clang/lib/Lex/HeaderSearch.cpp
+++ b/clang/lib/Lex/HeaderSearch.cpp
@@ -1408,57 +1408,57 @@ bool HeaderSearch::ShouldEnterIncludeFile(Preprocessor 
,
   // Get information about this file.
   HeaderFileInfo  = getFileInfo(File);
 
-  // FIXME: this is a workaround for the lack of proper modules-aware support
-  // for #import / #pragma once
-  auto TryEnterImported = [&]() -> bool {
-if (!ModulesEnabled)
-  return false;
-// Ensure FileInfo bits are up to date.
-ModMap.resolveHeaderDirectives(File);
-// Modules with builtins are special; multiple modules use builtins as
-// modular headers, example:
-//
-//module stddef { header "stddef.h" export * }
-//
-// After module map parsing, this expands to:
-//
-//module stddef {
-//  header "/path_to_builtin_dirs/stddef.h"
-//  textual "stddef.h"
-//}
-//
-// It's common that libc++ and system modules will both define such
-// submodules. Make sure cached results for a builtin header won't
-// prevent other builtin modules from potentially entering the builtin
-// header. Note that builtins are header guarded and the decision to
-// actually enter them is postponed to the controlling macros logic below.
-bool TryEnterHdr = false;
-if (FileInfo.isCompilingModuleHeader && FileInfo.isModuleHeader)
-  TryEnterHdr = ModMap.isBuiltinHeader(File);
-
-// Textual headers can be #imported from different modules. Since ObjC
-// headers find in the wild might rely only on #import and do not contain
-// controlling macros, be conservative and only try to enter textual 
headers
-// if such macro is present.
-if (!FileInfo.isModuleHeader &&
-FileInfo.getControllingMacro(ExternalLookup))
-  TryEnterHdr = true;
-return TryEnterHdr;
-  };
-
   // If this is a #import directive, check that we have not already imported
   // this header.
   if (isImport) {
 // If this has already been imported, don't import it again.
 FileInfo.isImport = true;
 
+// FIXME: this is a workaround for the lack of proper modules-aware support
+// for #import / #pragma once
+auto TryEnterImported = [&]() -> bool {
+  if (!ModulesEnabled)
+return false;
+  // Ensure FileInfo bits are up to date.
+  ModMap.resolveHeaderDirectives(File);
+  // Modules with builtins are special; multiple modules use builtins as
+  // modular headers, example:
+  //
+  //module stddef { header "stddef.h" export * }
+  //
+  // After module map parsing, this expands to:
+  //
+  //module stddef {
+  //  header "/path_to_builtin_dirs/stddef.h"
+  //  textual "stddef.h"
+  //}
+  //
+  // It's common that libc++ and system modules will both define such
+  // submodules. Make sure cached results for a builtin header won't
+  // prevent other builtin modules from potentially entering the builtin
+  // header. Note that builtins are header guarded and the decision to
+  // actually enter them is postponed to the controlling macros logic 
below.
+  bool TryEnterHdr = false;
+  if (FileInfo.isCompilingModuleHeader && FileInfo.isModuleHeader)
+TryEnterHdr = ModMap.isBuiltinHeader(File);
+
+  // Textual headers can be #imported from different modules. Since ObjC
+  // headers find in the wild might rely only on #import and do not contain
+  // controlling macros, be conservative and only try to enter textual
+  // headers if such macro is present.
+  if (!FileInfo.isModuleHeader &&
+  FileInfo.getControllingMacro(ExternalLookup))
+TryEnterHdr = true;
+  return TryEnterHdr;
+};
+
 // Has this already been #import'ed or #include'd?
 if (PP.alreadyIncluded(File) && 

[clang] ba1e84f - [C++20] [Modules] Handle inconsistent deduced function return type from importing modules

[Chuanqi Xu via cfe-commits]

Author: Chuanqi Xu
Date: 2024-01-23T16:19:51+08:00
New Revision: ba1e84fb8f45e102f40f409fcfe9b420fbf9fb70

URL: 
https://github.com/llvm/llvm-project/commit/ba1e84fb8f45e102f40f409fcfe9b420fbf9fb70
DIFF: 
https://github.com/llvm/llvm-project/commit/ba1e84fb8f45e102f40f409fcfe9b420fbf9fb70.diff

LOG: [C++20] [Modules] Handle inconsistent deduced function return type from 
importing modules

Close https://github.com/llvm/llvm-project/issues/78830
Close https://github.com/llvm/llvm-project/issues/60085

The direct reason of the issues is that in a module unit, the return
type of a function is deduced to a concrete type (e.g., int) but in the
other module unit, the return type of the same function is not deduced
yet (e.g, auto). Then when we importing the 2 modules, the later
function is selected but the code generator doesn't know how to generate
the auto type. So here is the crash.

The tricky part here is that, when the ASTReader reads the second
unreduced function, it finds the reading function has the same signature
with the already read deduced one and they have the same ODRHash. So
that the ASTReader skips loading the function body of the unreduced
function then the code generator can't infer the undeduced type like it
usually can. Also this is generally fine for functions without deducing
type since it is sufficient to emit a function call without the function
body.

Also in fact, we've already handled the case that the functon has
deduced type and its deducing state is inconsist in different modules:
https://github.com/llvm/llvm-project/blob/3ea92ea2f9d236569f82825cdba6d59bcc22495c/clang/lib/Serialization/ASTReader.cpp#L9531-L9544
and
https://github.com/llvm/llvm-project/blob/3ea92ea2f9d236569f82825cdba6d59bcc22495c/clang/lib/Serialization/ASTReaderDecl.cpp#L3643-L3647.

We've handled the case:
(1) If we read the undeduced functions first and read the deduced functions
later, the compiler will propagate the deduced type info for redecls in
the end of the reading.
(2) If we read the deduced functions first and read the undeduced
functions later, we will propagae the deduced type info when we **complete
the redecl chain**.

However, in the reporting issues, we're in the second case and
reproducer didn't trigger the action to complete the redecl chain. So
here is the crash.

Then it is obvious how should fix the problem. We should complete the
redecl chain for undeduced function types in the end of the reading for
the second case.

Added: 
clang/test/Modules/pr60085.cppm
clang/test/Modules/pr78830.cppm

Modified: 
clang/docs/ReleaseNotes.rst
clang/include/clang/Serialization/ASTReader.h
clang/lib/Serialization/ASTReader.cpp

Removed: 




diff  --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 93eecbe0e1363de..01c4ee97662b611 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -1049,6 +1049,11 @@ Bug Fixes to C++ Support
 
 - Set the ``__cpp_auto_cast`` feature test macro in C++23 mode.
 
+- Fix crash for inconsistent deducing state of function return types
+  in importing modules.
+  Fixes (`#78830 `_)
+  Fixes (`#60085 `_)
+
 Bug Fixes to AST Handling
 ^
 - Fixed an import failure of recursive friend class template.

diff  --git a/clang/include/clang/Serialization/ASTReader.h 
b/clang/include/clang/Serialization/ASTReader.h
index 21d791f5cd89a2e..dd1451bbf2d2c9f 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -550,6 +550,10 @@ class ASTReader
   /// declaration and the value is the deduced return type.
   llvm::SmallMapVector PendingDeducedTypeUpdates;
 
+  /// Functions has undededuced return type and we wish we can find the deduced
+  /// return type by iterating the redecls in other modules.
+  llvm::SmallVector PendingUndeducedFunctionDecls;
+
   /// Declarations that have been imported and have typedef names for
   /// linkage purposes.
   llvm::DenseMap, NamedDecl *>

diff  --git a/clang/lib/Serialization/ASTReader.cpp 
b/clang/lib/Serialization/ASTReader.cpp
index fe8782a3eb9e7cc..fecd94e875f671a 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -9534,12 +9534,21 @@ void ASTReader::finishPendingActions() {
   auto *FD = PendingDeducedFunctionTypes[I].first;
   FD->setType(GetType(PendingDeducedFunctionTypes[I].second));
 
-  // If we gave a function a deduced return type, remember that we need to
-  // propagate that along the redeclaration chain.
-  auto *DT = FD->getReturnType()->getContainedDeducedType();
-  if (DT && DT->isDeduced())
-PendingDeducedTypeUpdates.insert(
-{FD->getCanonicalDecl(), FD->getReturnType()});
+  if (auto *DT = 

[clang] [Serialization] Load Specializations Lazily (PR #76774)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 updated 
https://github.com/llvm/llvm-project/pull/76774

>From 26bd7dc5139811b2b0d8d8642a67b67340eeb1d5 Mon Sep 17 00:00:00 2001
From: Chuanqi Xu 
Date: Wed, 3 Jan 2024 11:33:17 +0800
Subject: [PATCH 1/4] Load Specializations Lazily

---
 clang/include/clang/AST/DeclTemplate.h|  35 ++--
 clang/include/clang/AST/ExternalASTSource.h   |   6 +
 clang/include/clang/AST/ODRHash.h |   3 +
 .../clang/Sema/MultiplexExternalSemaSource.h  |   6 +
 .../include/clang/Serialization/ASTBitCodes.h |   3 +
 clang/include/clang/Serialization/ASTReader.h |  28 +++
 clang/include/clang/Serialization/ASTWriter.h |   6 +
 clang/lib/AST/DeclTemplate.cpp|  67 +---
 clang/lib/AST/ExternalASTSource.cpp   |   5 +
 clang/lib/AST/ODRHash.cpp |   3 +
 .../lib/Sema/MultiplexExternalSemaSource.cpp  |   6 +
 clang/lib/Serialization/ASTReader.cpp | 125 +-
 clang/lib/Serialization/ASTReaderDecl.cpp |  35 +++-
 clang/lib/Serialization/ASTReaderInternals.h  |  80 +
 clang/lib/Serialization/ASTWriter.cpp | 144 +++-
 clang/lib/Serialization/ASTWriterDecl.cpp |  78 ++---
 clang/test/Modules/odr_hash.cpp   |   4 +-
 clang/unittests/Serialization/CMakeLists.txt  |   1 +
 .../Serialization/LoadSpecLazily.cpp  | 159 ++
 19 files changed, 729 insertions(+), 65 deletions(-)
 create mode 100644 clang/unittests/Serialization/LoadSpecLazily.cpp

diff --git a/clang/include/clang/AST/DeclTemplate.h 
b/clang/include/clang/AST/DeclTemplate.h
index 832ad2de6b08a82..4699dd17bc182cd 100644
--- a/clang/include/clang/AST/DeclTemplate.h
+++ b/clang/include/clang/AST/DeclTemplate.h
@@ -525,8 +525,11 @@ class FunctionTemplateSpecializationInfo final
 return Function.getInt();
   }
 
+  void loadExternalRedecls();
+
 public:
   friend TrailingObjects;
+  friend class ASTReader;
 
   static FunctionTemplateSpecializationInfo *
   Create(ASTContext , FunctionDecl *FD, FunctionTemplateDecl *Template,
@@ -789,13 +792,15 @@ class RedeclarableTemplateDecl : public TemplateDecl,
 return SpecIterator(isEnd ? Specs.end() : Specs.begin());
   }
 
-  void loadLazySpecializationsImpl() const;
+  void loadExternalSpecializations() const;
 
   template 
   typename SpecEntryTraits::DeclType*
   findSpecializationImpl(llvm::FoldingSetVector ,
  void *, ProfileArguments &&...ProfileArgs);
 
+  void loadLazySpecializationsWithArgs(ArrayRef 
TemplateArgs);
+
   template 
   void addSpecializationImpl(llvm::FoldingSetVector ,
  EntryType *Entry, void *InsertPos);
@@ -814,9 +819,13 @@ class RedeclarableTemplateDecl : public TemplateDecl,
 /// If non-null, points to an array of specializations (including
 /// partial specializations) known only by their external declaration IDs.
 ///
+/// These specializations needs to be loaded at once in
+/// loadExternalSpecializations to complete the redecl chain or be 
preparing
+/// for template resolution.
+///
 /// The first value in the array is the number of specializations/partial
 /// specializations that follow.
-uint32_t *LazySpecializations = nullptr;
+uint32_t *ExternalSpecializations = nullptr;
 
 /// The set of "injected" template arguments used within this
 /// template.
@@ -850,6 +859,8 @@ class RedeclarableTemplateDecl : public TemplateDecl,
   friend class ASTDeclWriter;
   friend class ASTReader;
   template  friend class RedeclarableTemplate;
+  friend class ClassTemplateSpecializationDecl;
+  friend class VarTemplateSpecializationDecl;
 
   /// Retrieves the canonical declaration of this template.
   RedeclarableTemplateDecl *getCanonicalDecl() override {
@@ -977,6 +988,7 @@ SpecEntryTraits {
 class FunctionTemplateDecl : public RedeclarableTemplateDecl {
 protected:
   friend class FunctionDecl;
+  friend class FunctionTemplateSpecializationInfo;
 
   /// Data that is common to all of the declarations of a given
   /// function template.
@@ -1012,13 +1024,13 @@ class FunctionTemplateDecl : public 
RedeclarableTemplateDecl {
   void addSpecialization(FunctionTemplateSpecializationInfo* Info,
  void *InsertPos);
 
+  /// Load any lazily-loaded specializations from the external source.
+  void LoadLazySpecializations() const;
+
 public:
   friend class ASTDeclReader;
   friend class ASTDeclWriter;
 
-  /// Load any lazily-loaded specializations from the external source.
-  void LoadLazySpecializations() const;
-
   /// Get the underlying function declaration of the template.
   FunctionDecl *getTemplatedDecl() const {
 return static_cast(TemplatedDecl);
@@ -1839,6 +1851,8 @@ class ClassTemplateSpecializationDecl
   LLVM_PREFERRED_TYPE(TemplateSpecializationKind)
   unsigned SpecializationKind : 3;
 
+  void loadExternalRedecls();
+
 protected:
   ClassTemplateSpecializationDecl(ASTContext , Kind DK, 

[clang] [Serialization] Load Specializations Lazily (PR #76774)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

In the newest push, I changed 2 things:
- In `ASTReader::LoadExternalSpecializations`, I am moving `Deserializing 
LookupResults(this);` before calculating the ODR hash for template arguments. 
Since I didn't realize that the ODRHash may trigger deserializing before.
- I introduced `-fno-load-external-specializations-lazily` to ease the 
debugging. When this option specified, all the specializations will be loaded 
when required, which keeps the previous behavior.

So if the new patch can pass directly, then it implies the issue is that I 
didn't realize the ODRHash may trigger deserializing.

And if not but it can pass with `-fno-load-external-specializations-lazily`, 
then it confirms the concern that the ODRHash algorithm for template arguments 
is not good.

Then if it can't pass with `-fno-load-external-specializations-lazily` too, it 
implies that I missed something : (

CC: @ilya-biryukov @hahnjo 

https://github.com/llvm/llvm-project/pull/76774
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Load Specializations Lazily (PR #76774)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 updated 
https://github.com/llvm/llvm-project/pull/76774

>From 26bd7dc5139811b2b0d8d8642a67b67340eeb1d5 Mon Sep 17 00:00:00 2001
From: Chuanqi Xu 
Date: Wed, 3 Jan 2024 11:33:17 +0800
Subject: [PATCH 1/4] Load Specializations Lazily

---
 clang/include/clang/AST/DeclTemplate.h|  35 ++--
 clang/include/clang/AST/ExternalASTSource.h   |   6 +
 clang/include/clang/AST/ODRHash.h |   3 +
 .../clang/Sema/MultiplexExternalSemaSource.h  |   6 +
 .../include/clang/Serialization/ASTBitCodes.h |   3 +
 clang/include/clang/Serialization/ASTReader.h |  28 +++
 clang/include/clang/Serialization/ASTWriter.h |   6 +
 clang/lib/AST/DeclTemplate.cpp|  67 +---
 clang/lib/AST/ExternalASTSource.cpp   |   5 +
 clang/lib/AST/ODRHash.cpp |   3 +
 .../lib/Sema/MultiplexExternalSemaSource.cpp  |   6 +
 clang/lib/Serialization/ASTReader.cpp | 125 +-
 clang/lib/Serialization/ASTReaderDecl.cpp |  35 +++-
 clang/lib/Serialization/ASTReaderInternals.h  |  80 +
 clang/lib/Serialization/ASTWriter.cpp | 144 +++-
 clang/lib/Serialization/ASTWriterDecl.cpp |  78 ++---
 clang/test/Modules/odr_hash.cpp   |   4 +-
 clang/unittests/Serialization/CMakeLists.txt  |   1 +
 .../Serialization/LoadSpecLazily.cpp  | 159 ++
 19 files changed, 729 insertions(+), 65 deletions(-)
 create mode 100644 clang/unittests/Serialization/LoadSpecLazily.cpp

diff --git a/clang/include/clang/AST/DeclTemplate.h 
b/clang/include/clang/AST/DeclTemplate.h
index 832ad2de6b08a8..4699dd17bc182c 100644
--- a/clang/include/clang/AST/DeclTemplate.h
+++ b/clang/include/clang/AST/DeclTemplate.h
@@ -525,8 +525,11 @@ class FunctionTemplateSpecializationInfo final
 return Function.getInt();
   }
 
+  void loadExternalRedecls();
+
 public:
   friend TrailingObjects;
+  friend class ASTReader;
 
   static FunctionTemplateSpecializationInfo *
   Create(ASTContext , FunctionDecl *FD, FunctionTemplateDecl *Template,
@@ -789,13 +792,15 @@ class RedeclarableTemplateDecl : public TemplateDecl,
 return SpecIterator(isEnd ? Specs.end() : Specs.begin());
   }
 
-  void loadLazySpecializationsImpl() const;
+  void loadExternalSpecializations() const;
 
   template 
   typename SpecEntryTraits::DeclType*
   findSpecializationImpl(llvm::FoldingSetVector ,
  void *, ProfileArguments &&...ProfileArgs);
 
+  void loadLazySpecializationsWithArgs(ArrayRef 
TemplateArgs);
+
   template 
   void addSpecializationImpl(llvm::FoldingSetVector ,
  EntryType *Entry, void *InsertPos);
@@ -814,9 +819,13 @@ class RedeclarableTemplateDecl : public TemplateDecl,
 /// If non-null, points to an array of specializations (including
 /// partial specializations) known only by their external declaration IDs.
 ///
+/// These specializations needs to be loaded at once in
+/// loadExternalSpecializations to complete the redecl chain or be 
preparing
+/// for template resolution.
+///
 /// The first value in the array is the number of specializations/partial
 /// specializations that follow.
-uint32_t *LazySpecializations = nullptr;
+uint32_t *ExternalSpecializations = nullptr;
 
 /// The set of "injected" template arguments used within this
 /// template.
@@ -850,6 +859,8 @@ class RedeclarableTemplateDecl : public TemplateDecl,
   friend class ASTDeclWriter;
   friend class ASTReader;
   template  friend class RedeclarableTemplate;
+  friend class ClassTemplateSpecializationDecl;
+  friend class VarTemplateSpecializationDecl;
 
   /// Retrieves the canonical declaration of this template.
   RedeclarableTemplateDecl *getCanonicalDecl() override {
@@ -977,6 +988,7 @@ SpecEntryTraits {
 class FunctionTemplateDecl : public RedeclarableTemplateDecl {
 protected:
   friend class FunctionDecl;
+  friend class FunctionTemplateSpecializationInfo;
 
   /// Data that is common to all of the declarations of a given
   /// function template.
@@ -1012,13 +1024,13 @@ class FunctionTemplateDecl : public 
RedeclarableTemplateDecl {
   void addSpecialization(FunctionTemplateSpecializationInfo* Info,
  void *InsertPos);
 
+  /// Load any lazily-loaded specializations from the external source.
+  void LoadLazySpecializations() const;
+
 public:
   friend class ASTDeclReader;
   friend class ASTDeclWriter;
 
-  /// Load any lazily-loaded specializations from the external source.
-  void LoadLazySpecializations() const;
-
   /// Get the underlying function declaration of the template.
   FunctionDecl *getTemplatedDecl() const {
 return static_cast(TemplatedDecl);
@@ -1839,6 +1851,8 @@ class ClassTemplateSpecializationDecl
   LLVM_PREFERRED_TYPE(TemplateSpecializationKind)
   unsigned SpecializationKind : 3;
 
+  void loadExternalRedecls();
+
 protected:
   ClassTemplateSpecializationDecl(ASTContext , Kind DK, 

[clang] [Serialization] Load Specializations Lazily (PR #76774)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> > > > I tried applying this patch to ROOT/Cling and it fails to build because 
> > > > something is of the opinion that `std::is_integral::value` is not 
> > > > true. I don't have time right now to investigate further, but since 
> > > > this is the only change I did it is highly likely that it's caused by 
> > > > this...
> > > 
> > > 
> > > In that case we need to understand how this patch differs from the one I 
> > > pasted in phabricator since the one there works for that case I assume.
> > 
> > 
> > I had a guess. `{ClassTemplateDecl, VarTemplateDecl, 
> > FunctionTemplateDecl}::specializations()` will iterate all the 
> > specializations. However, after this patch, there are maybe some 
> > specializations not loaded at the time of invocation. Then during the 
> > process of iterating `specializations()`, it may require to complete the 
> > redecl chain to trigger the process of loading specializations. Then boom!
> 
> This is also the case with https://reviews.llvm.org/D41416 and we have 
> machinery to deal with that (including https://reviews.llvm.org/D154328)

Got it. So it is not the cause for the problem you saw : (

Would you like to give an reproducer so that I can debug?

https://github.com/llvm/llvm-project/pull/76774
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] a31a600 - [docs] Update StandardCPlusPlusModules.rst with clang18

[Chuanqi Xu via cfe-commits]

Author: Chuanqi Xu
Date: 2024-01-22T14:24:33+08:00
New Revision: a31a60074717fc40887cfe132b77eec93bedd307

URL: 
https://github.com/llvm/llvm-project/commit/a31a60074717fc40887cfe132b77eec93bedd307
DIFF: 
https://github.com/llvm/llvm-project/commit/a31a60074717fc40887cfe132b77eec93bedd307.diff

LOG: [docs] Update StandardCPlusPlusModules.rst with clang18

Changed Things:
- Mentioning we need to specify BMIs for indirectly dependent BMIs too.
- Remove the note for `delayed-template-parsing` since
  https://github.com/llvm/llvm-project/issues/61068 got closed.
- Add a note for https://github.com/llvm/llvm-project/issues/78850 since
  we've seen it for a lot of times.
- Add a note for https://github.com/llvm/llvm-project/issues/78173 since
  we've seen it for a lot of times.

Added: 


Modified: 
clang/docs/StandardCPlusPlusModules.rst

Removed: 




diff  --git a/clang/docs/StandardCPlusPlusModules.rst 
b/clang/docs/StandardCPlusPlusModules.rst
index 22d506f0da2b10..81043ff25be02e 100644
--- a/clang/docs/StandardCPlusPlusModules.rst
+++ b/clang/docs/StandardCPlusPlusModules.rst
@@ -345,6 +345,9 @@ In case all ``-fprebuilt-module-path=``, 
``-fmodule-file==`` 
will take the second
 highest precedence.
 
+We need to specify all the dependent (directly and indirectly) BMIs.
+See https://github.com/llvm/llvm-project/issues/62707 for detail.
+
 When we compile a ``module implementation unit``, we must specify the BMI of 
the corresponding
 ``primary module interface unit``.
 Since the language specification says a module implementation unit implicitly 
imports
@@ -689,14 +692,68 @@ the BMI within ``clang-cl.exe``.
 
 This is tracked in: https://github.com/llvm/llvm-project/issues/64118
 
-delayed template parsing is not supported/broken with C++ modules
-~
+false positive ODR violation diagnostic due to using inconsistent qualified 
but the same type
+~
+
+ODR violation is a pretty common issue when using modules.
+Sometimes the program violated the One Definition Rule actually.
+But sometimes it shows the compiler gives false positive diagnostics.
+
+One often reported example is:
+
+.. code-block:: c++
+
+  // part.cc
+  module;
+  typedef long T;
+  namespace ns {
+  inline void fun() {
+  (void)(T)0;
+  }
+  }
+  export module repro:part;
+
+  // repro.cc
+  module;
+  typedef long T;
+  namespace ns {
+  using ::T;
+  }
+  namespace ns {
+  inline void fun() {
+  (void)(T)0;
+  }
+  }
+  export module repro;
+  export import :part;
+
+Currently the compiler complains about the inconsistent definition of `fun()` 
in
+2 module units. This is incorrect. Since both definitions of `fun()` has the 
same
+spelling and `T` refers to the same type entity finally. So the program should 
be
+fine.
+
+This is tracked in https://github.com/llvm/llvm-project/issues/78850.
+
+Using TU-local entity in other units
+
+
+Module units are translation units. So the entities which should only be local 
to the
+module unit itself shouldn't be used by other units in any means.
+
+In the language side, to address the idea formally, the language specification 
defines
+the concept of ``TU-local`` and ``exposure`` in
+`basic.link/p14 `_,
+`basic.link/p15 `_,
+`basic.link/p16 `_,
+`basic.link/p17 `_ and
+`basic.link/p18 `_.
 
-The feature `-fdelayed-template-parsing` can't work well with C++ modules now.
-Note that this is significant on Windows since the option will be enabled by 
default
-on Windows.
+However, the compiler doesn't support these 2 ideas formally.
+This results in unclear and confusing diagnostic messages.
+And it is worse that the compiler may import TU-local entities to other units 
without any
+diagnostics.
 
-This is tracked in: https://github.com/llvm/llvm-project/issues/61068
+This is tracked in https://github.com/llvm/llvm-project/issues/78173.
 
 Header Units
 



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Load Specializations Lazily (PR #76774)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> > I tried applying this patch to ROOT/Cling and it fails to build because 
> > something is of the opinion that `std::is_integral::value` is not 
> > true. I don't have time right now to investigate further, but since this is 
> > the only change I did it is highly likely that it's caused by this...
> 
> In that case we need to understand how this patch differs from the one I 
> pasted in phabricator since the one there works for that case I assume.

I had a guess. `{ClassTemplateDecl, VarTemplateDecl, 
FunctionTemplateDecl}::specializations()` will iterate all the specializations. 
However, after this patch, there are maybe some specializations not loaded at 
the time of invocation. Then during the process of iterating 
`specializations()`, it may require to complete the redecl chain to trigger the 
process of loading specializations 
(https://github.com/llvm/llvm-project/blob/2e30e31e1e80184d9b2c8aa98f617b4d1cb56d55/clang/include/clang/AST/DeclTemplate.h#L779-L781).
 Then boom!

To fix this, maybe we need to add a `loadAllExternalSpecializations(Decl*)` 
method to be called before `{ClassTemplateDecl, VarTemplateDecl, 
FunctionTemplateDecl}::specializations()`. But I am not sure if it is really 
the case and I feel better if we can add new things with the corresponding test.

https://github.com/llvm/llvm-project/pull/76774
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Load Specializations Lazily (PR #76774)

[Chuanqi Xu via cfe-commits]

@@ -3924,6 +3925,154 @@ class ASTDeclContextNameLookupTrait {
 
 } // namespace
 
+namespace {
+class SpecializationsLookupTrait {
+  ASTWriter 
+  llvm::SmallVector DeclIDs;
+
+public:
+  using key_type = unsigned;
+  using key_type_ref = key_type;
+
+  /// A start and end index into DeclIDs, representing a sequence of decls.
+  using data_type = std::pair;
+  using data_type_ref = const data_type &;
+
+  using hash_value_type = unsigned;
+  using offset_type = unsigned;
+
+  explicit SpecializationsLookupTrait(ASTWriter ) : Writer(Writer) {}
+
+  template  data_type getData(Col &) {
+unsigned Start = DeclIDs.size();
+for (auto *D : C)
+  DeclIDs.push_back(Writer.GetDeclRef(getDeclForLocalLookup(
+  Writer.getLangOpts(), const_cast(D;
+return std::make_pair(Start, DeclIDs.size());
+  }
+
+  data_type
+  ImportData(const reader::SpecializationsLookupTrait::data_type ) {
+unsigned Start = DeclIDs.size();
+for (auto ID : FromReader)
+  DeclIDs.push_back(ID);
+return std::make_pair(Start, DeclIDs.size());
+  }
+
+  static bool EqualKey(key_type_ref a, key_type_ref b) { return a == b; }
+
+  hash_value_type ComputeHash(key_type Name) { return Name; }
+
+  void EmitFileRef(raw_ostream , ModuleFile *F) const {
+assert(Writer.hasChain() &&
+   "have reference to loaded module file but no chain?");
+
+using namespace llvm::support;
+endian::write(Out, Writer.getChain()->getModuleFileID(F),
+llvm::endianness::little);
+  }
+
+  std::pair EmitKeyDataLength(raw_ostream ,
+  key_type HashValue,
+  data_type_ref Lookup) {
+// 4 bytes for each slot.
+unsigned KeyLen = 4;
+unsigned DataLen = 4 * (Lookup.second - Lookup.first);
+
+return emitULEBKeyDataLength(KeyLen, DataLen, Out);
+  }
+
+  void EmitKey(raw_ostream , key_type HashValue, unsigned) {
+using namespace llvm::support;
+
+endian::Writer LE(Out, llvm::endianness::little);
+LE.write(HashValue);
+  }
+
+  void EmitData(raw_ostream , key_type_ref, data_type Lookup,
+unsigned DataLen) {
+using namespace llvm::support;
+
+endian::Writer LE(Out, llvm::endianness::little);
+uint64_t Start = Out.tell();
+(void)Start;
+for (unsigned I = Lookup.first, N = Lookup.second; I != N; ++I)
+  LE.write(DeclIDs[I]);
+assert(Out.tell() - Start == DataLen && "Data length is wrong");
+  }
+};
+
+unsigned CalculateODRHashForSpecs(const Decl *Spec) {

ChuanqiXu9 wrote:

It looks like the implementation is basically the same with 
https://reviews.llvm.org/D41416# except this patch added the size of templated 
args, which shouldn't matter.

https://github.com/llvm/llvm-project/pull/76774
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 407db48 - [Release Notes] [C++20] [Modules] Summarize modules related change to the release note for clang 18

[Chuanqi Xu via cfe-commits]

Author: Chuanqi Xu
Date: 2024-01-19T15:26:22+08:00
New Revision: 407db48eb4d387ae272bfbef9b12868806f1e830

URL: 
https://github.com/llvm/llvm-project/commit/407db48eb4d387ae272bfbef9b12868806f1e830
DIFF: 
https://github.com/llvm/llvm-project/commit/407db48eb4d387ae272bfbef9b12868806f1e830.diff

LOG: [Release Notes] [C++20] [Modules] Summarize modules related change to the 
release note for clang 18

Given clang18 is going to be released, this patch collects the modules
related change to release notes to make users have a clear
vision for modules.

Added: 


Modified: 
clang/docs/ReleaseNotes.rst

Removed: 




diff  --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 63f756f30873c6..392f694065a242 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -97,10 +97,6 @@ C/C++ Language Potentially Breaking Changes
   outlined in "The Equality Operator You Are Looking For" (`P2468 
`_).
   Fixes (`#68901: `_).
 
-- Remove the hardcoded path to the imported modules for C++20 named modules. 
Now we
-  require all the dependent modules to specified from the command line.
-  See (`#62707: `_).
-
 C++ Specific Potentially Breaking Changes
 -
 - The name mangling rules for function templates has been changed to take into
@@ -141,6 +137,13 @@ C++ Specific Potentially Breaking Changes
   when targetting MSVC to match the behavior of MSVC.
   (`MSVC Docs 
`_)
 
+- Remove the hardcoded path to the imported modules for C++20 named modules. 
Now we
+  require all the dependent modules to specified from the command line.
+  See (`#62707: `_).
+
+- Forbid `import XXX;` in C++ to find module `XXX` comes from explicit clang 
modules.
+  See (`#64755: `_).
+
 ABI Changes in This Version
 ---
 - Following the SystemV ABI for x86-64, ``__int128`` arguments will no longer
@@ -596,6 +599,17 @@ Improvements to Clang's diagnostics
13 | new (__builtin_memset) S {};
   |  ^
 
+- Clang now diagnoses import before module declarations but not in global
+  module fragment.
+  (`#67627: `_).
+
+- Clang now diagnoses include headers with angle in module purviews, which is
+  not usually intended.
+  (`#68615: `_)
+
+- Clang now won't mention invisible namespace when diagnose invisible 
declarations
+  inside namespace. The original diagnostic message is confusing.
+  (`#73893: `_)
 
 Improvements to Clang's time-trace
 --
@@ -982,6 +996,16 @@ Bug Fixes to C++ Support
 - Clang now allows parenthesized initialization of arrays in `operator new[]`.
   Fixes: (`#68198 `_)
 
+- Fix crash when importing the same module with an dynamic initializer twice
+  in 
diff erent visibility.
+  Fixes (`#67893 `_)
+
+- Fix a false-positive ODR violation for 
diff erent definitions for `std::align_val_t`.
+  Fixes (`#76638 `_)
+
+- Remove recorded `#pragma once` state for headers included in named modules.
+  Fixes (`#77995 `_)
+
 Bug Fixes to AST Handling
 ^
 - Fixed an import failure of recursive friend class template.
@@ -1350,6 +1374,14 @@ Improvements
 - Checkers can query constraint bounds to improve diagnostic messages.
   (`#74141 `_)
 
+- Improved the generated initializers for modules. Now the calls to initialize
+  functions from imported module units can be omitted if the initialize
+  function is known to be empty.
+  (`#56794 `_)
+
+- Clang now allow to export declarations within linkage-specification.
+  (`#71347 `_)
+
 Moved checkers
 ^^
 



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Load Specializations Lazily (PR #76774)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> We saw some failures internally with this patch:
> 
> * `"'absl::AnyInvocable' has different definitions in different 
> modules;".`
>   Probably related to the template arguments stability you mention.
> * `"No type named 'MemberType' in 'some_object::Traits'"`.
>   I suspect that's exposing some bug in modules that we were lucky to avoid 
> before.
> * `'std::pointer_traits::pointer_to' from 
> module '/optional' is not present in definition of 
> 'std::pointer_traits' provided earlier`
>   Probably the same root cause as the previous item, but shows up as a 
> different error message (although it does point out that we might have some 
> diamond-dependency-style problem here).
> * Code of the form
> 
> ```c++
> auto* stream = new (arena_alloc(param.call->call())) Callback();
> param.context->Begin([stream](bool) { stream->Done(); });
> ```
> 
> produces really weird error (as if lambda captured something from the line 
> **prior to lambda**):
> 
> ```shell
> error: variable 'param' cannot be implicitly captured in a lambda with no 
> capture-default specified
>   679 | auto* stream = new (arena_alloc(param.call->call())) 
> Callback();
>   | ^
> note: while substituting into a lambda expression here
>   680 |   param.context->Begin([stream](bool) { stream->Done(); });
> ```
> 
> Looks unexpected. I suspect that it's a different issue (because of the way 
> we test these things, we also include some extra commits from head that could 
> have additional problems). However, I still wanted to mention it in case 
> people here think it might be related.
> 
> * Crashes. They need more investigation.
> 
> We will to provide reproducers as soon as possible, but please bear with us 
> for some time as module-related issues are very hard to minimize and share as 
> a small code example due to lack of infrastructure. They normally happen on 
> complicated build graphs with too many dependencies to easily reduce. 
> However, I am optimistic at least about the template arguments issue because 
> it seems to happen at `absl` layer, which has relatively few dependencies.

Thanks for the high-quality reports. Looking forward to the reduced case : )

https://github.com/llvm/llvm-project/pull/76774
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [coroutine] Create coroutine body in the correct eval context (PR #78589)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 approved this pull request.

LGTM. Thanks.

https://github.com/llvm/llvm-project/pull/78589
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 085eae6 - [C++20] [Modules] Allow to merge enums with the same underlying interger types

[Chuanqi Xu via cfe-commits]

Author: Chuanqi Xu
Date: 2024-01-18T17:09:35+08:00
New Revision: 085eae6b863881fb9fda323e5b672b04a00ed19e

URL: 
https://github.com/llvm/llvm-project/commit/085eae6b863881fb9fda323e5b672b04a00ed19e
DIFF: 
https://github.com/llvm/llvm-project/commit/085eae6b863881fb9fda323e5b672b04a00ed19e.diff

LOG: [C++20] [Modules] Allow to merge enums with the same underlying interger 
types

Close https://github.com/llvm/llvm-project/issues/76638. See the issue
for the context of the change.

Added: 
clang/test/Modules/pr76638.cppm

Modified: 
clang/lib/AST/ODRHash.cpp

Removed: 




diff  --git a/clang/lib/AST/ODRHash.cpp b/clang/lib/AST/ODRHash.cpp
index aea1a93ae1fa828..07677d655c5afd6 100644
--- a/clang/lib/AST/ODRHash.cpp
+++ b/clang/lib/AST/ODRHash.cpp
@@ -741,8 +741,55 @@ void ODRHash::AddEnumDecl(const EnumDecl *Enum) {
   if (Enum->isScoped())
 AddBoolean(Enum->isScopedUsingClassTag());
 
-  if (Enum->getIntegerTypeSourceInfo())
-AddQualType(Enum->getIntegerType());
+  if (Enum->getIntegerTypeSourceInfo()) {
+// FIMXE: This allows two enums with 
diff erent spellings to have the same
+// hash.
+//
+//  // mod1.cppm
+//  module;
+//  extern "C" {
+//  typedef unsigned __int64 size_t;
+//  }
+//  namespace std {
+//  using :: size_t;
+//  }
+//
+//  extern "C++" {
+//  namespace std {
+//  enum class align_val_t : std::size_t {};
+//  }
+//  }
+//
+//  export module mod1;
+//  export using std::align_val_t;
+//
+//  // mod2.cppm
+//  module;
+//  extern "C" {
+//  typedef unsigned __int64 size_t;
+//  }
+//
+//  extern "C++" {
+//  namespace std {
+//  enum class align_val_t : size_t {};
+//  }
+//  }
+//
+//  export module mod2;
+//  import mod1;
+//  export using std::align_val_t;
+//
+// The above example should be disallowed since it violates
+// [basic.def.odr]p14:
+//
+//Each such definition shall consist of the same sequence of tokens
+//
+// The definitions of `std::align_val_t` in two module units have 
diff erent
+// spellings but we failed to give an error here.
+//
+// See https://github.com/llvm/llvm-project/issues/76638 for details.
+AddQualType(Enum->getIntegerType().getCanonicalType());
+  }
 
   // Filter out sub-Decls which will not be processed in order to get an
   // accurate count of Decl's.

diff  --git a/clang/test/Modules/pr76638.cppm b/clang/test/Modules/pr76638.cppm
new file mode 100644
index 000..8cc807961421b7c
--- /dev/null
+++ b/clang/test/Modules/pr76638.cppm
@@ -0,0 +1,69 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/mod1.cppm -emit-module-interface -o 
%t/mod1.pcm
+// RUN: %clang_cc1 -std=c++20 %t/mod2.cppm -fmodule-file=mod1=%t/mod1.pcm \
+// RUN: -fsyntax-only -verify
+//
+// RUN: %clang_cc1 -std=c++20 %t/mod3.cppm -emit-module-interface -o 
%t/mod3.pcm
+// RUN: %clang_cc1 -std=c++20 %t/mod4.cppm -fmodule-file=mod3=%t/mod3.pcm \
+// RUN: -fsyntax-only -verify
+
+//--- size_t.h
+
+extern "C" {
+typedef unsigned int size_t;
+}
+
+//--- csize_t
+namespace std {
+using :: size_t;
+}
+
+//--- align.h
+namespace std {
+enum class align_val_t : size_t {};
+}
+
+//--- mod1.cppm
+module;
+#include "size_t.h"
+#include "align.h"
+export module mod1;
+export using std::align_val_t;
+
+//--- mod2.cppm
+// expected-no-diagnostics
+module;
+#include "size_t.h"
+#include "csize_t"
+#include "align.h"
+export module mod2;
+import mod1;
+export using std::align_val_t;
+
+//--- signed_size_t.h
+// Test that we can still find the case if the underlying type is 
diff erent
+extern "C" {
+typedef signed int size_t;
+}
+
+//--- mod3.cppm
+module;
+#include "size_t.h"
+#include "align.h"
+export module mod3;
+export using std::align_val_t;
+
+//--- mod4.cppm
+module;
+#include "signed_size_t.h"
+#include "csize_t"
+#include "align.h"
+export module mod4;
+import mod3;
+export using std::align_val_t;
+
+// expected-error@align.h:* {{'std::align_val_t' has 
diff erent definitions in 
diff erent modules; defined here first 
diff erence is enum with specified type 'size_t' (aka 'int')}}
+// expected-note@align.h:* {{but in 'mod3.' found enum with specified 
type 'size_t' (aka 'unsigned int')}}



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Warning for incorrect useof 'pure' attribute (PR #78200)

[Chuanqi Xu via cfe-commits]

@@ -11802,6 +11802,27 @@ static bool CheckMultiVersionFunction(Sema , 
FunctionDecl *NewFD,
  OldDecl, Previous);
 }
 
+static void CheckFunctionDeclarationAttributesUsage(Sema ,
+FunctionDecl *NewFD) {
+  bool IsPure = NewFD->hasAttr();
+  bool IsConst = NewFD->hasAttr();
+
+  if (IsPure && IsConst) {
+S.Diag(NewFD->getLocation(), diag::warn_const_attr_with_pure_attr);
+NewFD->dropAttr();
+  }
+  if (IsPure || IsConst) {

ChuanqiXu9 wrote:

Then we should handle different attributes in different places.

https://github.com/llvm/llvm-project/pull/78200
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Warning for incorrect useof 'pure' attribute (PR #78200)

[Chuanqi Xu via cfe-commits]

@@ -692,6 +692,13 @@ def warn_maybe_falloff_nonvoid_function : Warning<
 def warn_falloff_nonvoid_function : Warning<
   "non-void function does not return a value">,
   InGroup;
+def warn_const_attr_with_pure_attr : Warning<
+  "'const' attribute imposes more restrictions, 'pure' attribute ignored">,
+  InGroup;

ChuanqiXu9 wrote:

While this is not required, **incorrect** is too broad. e.g., a lot of clang's 
change  can be summarized as fixing incorrect  implementation for C++.

https://github.com/llvm/llvm-project/pull/78200
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Warning for incorrect useof 'pure' attribute (PR #78200)

[Chuanqi Xu via cfe-commits]

@@ -11802,6 +11802,27 @@ static bool CheckMultiVersionFunction(Sema , 
FunctionDecl *NewFD,
  OldDecl, Previous);
 }
 
+static void CheckFunctionDeclarationAttributesUsage(Sema ,
+FunctionDecl *NewFD) {
+  bool IsPure = NewFD->hasAttr();
+  bool IsConst = NewFD->hasAttr();
+
+  if (IsPure && IsConst) {
+S.Diag(NewFD->getLocation(), diag::warn_const_attr_with_pure_attr);
+NewFD->dropAttr();
+  }
+  if (IsPure || IsConst) {

ChuanqiXu9 wrote:

```suggestion
  if (!IsPure && !IsConst)
 return;
   ...
```

nit: we prefer short indentation.



https://github.com/llvm/llvm-project/pull/78200
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Warning for incorrect useof 'pure' attribute (PR #78200)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 approved this pull request.

LGTM

https://github.com/llvm/llvm-project/pull/78200
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Warning for incorrect useof 'pure' attribute (PR #78200)

[Chuanqi Xu via cfe-commits]

@@ -692,6 +692,13 @@ def warn_maybe_falloff_nonvoid_function : Warning<
 def warn_falloff_nonvoid_function : Warning<
   "non-void function does not return a value">,
   InGroup;
+def warn_const_attr_with_pure_attr : Warning<
+  "'const' attribute imposes more restrictions, 'pure' attribute ignored">,
+  InGroup;

ChuanqiXu9 wrote:

nit: generally we prefer one patch does one thing manner. So maybe we can add 
this in later patches.

https://github.com/llvm/llvm-project/pull/78200
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Warning for incorrect useof 'pure' attribute (PR #78200)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 edited 
https://github.com/llvm/llvm-project/pull/78200
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Print library module manifest path. (PR #76451)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> > LGTM.
> > We need to delete 
> > `clang/test/Driver/Inputs/cxx23_modules/usr/lib/x86_64-linux-gnu/libc++.so` 
> > and 
> > `clang/test/Driver/Inputs/cxx23_modules/usr/lib/x86_64-linux-gnu/modules.json`,
> >  we should generate them with `split-file`
> 
> Are you sure that is the proper way? Should these tests not use a complete 
> installation to make sure it works on installations?

We can use `mkdir` to generate more complex paths. For example,

```
// RUN: split-file %s %t
//
// RUN: mkdir %t/
// RUN: touch %t/.../libc++.so
```


The intention is to remove the use of `Inputs` in test. It is really hard to 
read.

https://github.com/llvm/llvm-project/pull/76451
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [coroutines][coro_lifetimebound] Detect lifetime issues with lambda captures (PR #77066)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 approved this pull request.

LGTM then

https://github.com/llvm/llvm-project/pull/77066
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [coroutines][coro_lifetimebound] Detect lifetime issues with lambda captures (PR #77066)

[Chuanqi Xu via cfe-commits]

@@ -15845,8 +15845,10 @@ void Sema::CheckCoroutineWrapper(FunctionDecl *FD) {
   RecordDecl *RD = FD->getReturnType()->getAsRecordDecl();
   if (!RD || !RD->getUnderlyingDecl()->hasAttr())
 return;
-  // Allow `get_return_object()`.
-  if (FD->getDeclName().isIdentifier() &&
+  // Allow some_promise_type::get_return_object().
+  // Since we are still in the promise definition, we can only do this
+  // heuristically as the promise may not be yet associated to a coroutine.
+  if (isa(FD) && FD->getDeclName().isIdentifier() &&
   FD->getName().equals("get_return_object") && FD->param_empty())
 return;

ChuanqiXu9 wrote:

> I do not think that would work either. For example, consider TUs with only 
> promise definitions and no coroutine definitions. These attr would not be 
> available even in the end of TU.

Oh, yeah. I didn't think about modules : ).

>  @ChuanqiXu9 is it fine to add attributes to existing functions conditioned 
> on how they are used rather than how they are defined?

Then it is troublesome to use this attribute, which violates our intention.

https://github.com/llvm/llvm-project/pull/77066
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 1b6c1a3 - [NFC] Improve test for clang/test/Modules/GH77953.cpp

[Chuanqi Xu via cfe-commits]

Author: Chuanqi Xu
Date: 2024-01-16T11:32:10+08:00
New Revision: 1b6c1a3bd73be4dd904230c637d65810cf3334cd

URL: 
https://github.com/llvm/llvm-project/commit/1b6c1a3bd73be4dd904230c637d65810cf3334cd
DIFF: 
https://github.com/llvm/llvm-project/commit/1b6c1a3bd73be4dd904230c637d65810cf3334cd.diff

LOG: [NFC] Improve test for clang/test/Modules/GH77953.cpp

Generally we'll use `-fsyntax-only` when it is sufficient and we'll use
`expected-no-diagnostics` to test the behavior doesn't trigger any
problems. This patch applies these two improvements to
`clang/test/Modules/GH77953.cpp`.

Added: 


Modified: 
clang/test/Modules/GH77953.cpp

Removed: 




diff  --git a/clang/test/Modules/GH77953.cpp b/clang/test/Modules/GH77953.cpp
index aaca8153c3d212..c0c16f00395bce 100644
--- a/clang/test/Modules/GH77953.cpp
+++ b/clang/test/Modules/GH77953.cpp
@@ -4,7 +4,7 @@
 // RUN: split-file %s %t
 
 // RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/a.cppm -o %t/a.pcm
-// RUN: %clang_cc1 -std=c++20 -fmodule-file=a=%t/a.pcm %t/b.cppm
+// RUN: %clang_cc1 -std=c++20 -fmodule-file=a=%t/a.pcm %t/b.cpp -fsyntax-only 
-verify
 
 //--- a.cppm
 export module a;
@@ -22,7 +22,7 @@ struct a {
 
 template struct a<>;
 
-//--- b.cppm
+//--- b.cpp
+// expected-no-diagnostics
 import a;
-
 template struct a;



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] Only compare template params of potential overload after checking their decl context (PR #78139)

[Chuanqi Xu via cfe-commits]

@@ -0,0 +1,28 @@
+// From https://github.com/llvm/llvm-project/issues/77953
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/a.cppm -o %t/a.pcm
+// RUN: %clang_cc1 -std=c++20 -fmodule-file=a=%t/a.pcm %t/b.cppm

ChuanqiXu9 wrote:

nit: `%clang_cc1 -std=c++20 -fmodule-file=a=%t/a.pcm %t/b.cpp -fsyntax-only 
-verify`

I didn't look this in time. I'll try to improve this in a seperate NFC commit.

https://github.com/llvm/llvm-project/pull/78139
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Load Specializations Lazily (PR #76774)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> > @ilya-biryukov any chance you/your folks could test this change for 
> > performance implications in google? It's especially helpful to CERN, but 
> > the last iteration of this direction had some regressions that stalled out 
> > progress on that version a few years ago, so it'd be good to help poke this 
> > along while making sure it doesn't cause release hiccups/etc for google.
> 
> Sorry for the long reply time. I will check which ways to check compilation 
> performance we have here. I believe we should also probably check this for 
> correctness (which should be slightly easier than checking the performance 
> implications) as we are hitting quite some issues related to lazy reads from 
> modules, which have proven very hard to debug and reduce in the last years. I 
> am slightly worried it may get worse with this change and we won't be able to 
> compile our code because of some other existing bugs.

I feel it may be sufficient to test the correctness by running your internal 
workloads. And if it compiles and passes the test, we can assume it is correct.

And for the debugability, yeah, it is a problem. But it has been a problem for 
a long time. To not make it worse with the patch, maybe we can add a CC1 flag 
for the patch to always emit the same hash value for template arguments. Then 
it becomes to the same behavior with the current one. 

Further more, for the debugability, I am wondering if we can make it better by 
dumpping all the deserialized declarations (this is easy with deserialization 
listener) well. This may be helpful since we can get rid of all the unused 
things immediately.

https://github.com/llvm/llvm-project/pull/76774
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Warning for incorrect useof 'pure' attribute (PR #78200)

[Chuanqi Xu via cfe-commits]

@@ -11889,6 +11889,13 @@ bool Sema::CheckFunctionDeclaration(Scope *S, 
FunctionDecl *NewFD,
 NewFD->setInvalidDecl();
   }
 
+  if (NewFD->hasAttr() || NewFD->hasAttr()) {
+if (isa(NewFD))
+  Diag(NewFD->getLocation(), diag::warn_pure_attr_on_cxx_constructor);
+else if (NewFD->getReturnType()->isVoidType())

ChuanqiXu9 wrote:

Also let's get some comments for the rationale here even if you feel it is 
obivious.

https://github.com/llvm/llvm-project/pull/78200
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Warning for incorrect useof 'pure' attribute (PR #78200)

[Chuanqi Xu via cfe-commits]

@@ -692,6 +692,13 @@ def warn_maybe_falloff_nonvoid_function : Warning<
 def warn_falloff_nonvoid_function : Warning<
   "non-void function does not return a value">,
   InGroup;
+def warn_pure_attr_on_cxx_constructor : Warning<
+  "constructor cannot be 'pure' (undefined behavior)">,
+  InGroup;

ChuanqiXu9 wrote:

```suggestion
  InGroup<>;
```

It looks a little bit far to invent `IncorrectAttributeUsage` group here

https://github.com/llvm/llvm-project/pull/78200
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Warning for incorrect useof 'pure' attribute (PR #78200)

[Chuanqi Xu via cfe-commits]

@@ -11889,6 +11889,13 @@ bool Sema::CheckFunctionDeclaration(Scope *S, 
FunctionDecl *NewFD,
 NewFD->setInvalidDecl();
   }
 
+  if (NewFD->hasAttr() || NewFD->hasAttr()) {
+if (isa(NewFD))
+  Diag(NewFD->getLocation(), diag::warn_pure_attr_on_cxx_constructor);
+else if (NewFD->getReturnType()->isVoidType())
+  Diag(NewFD->getLocation(), diag::warn_pure_function_returns_void);

ChuanqiXu9 wrote:

Then the title and the diagnostic might be confusing if the user are using 
`[[gnu::const]]`.  You can look at the use of `%select` in 
`clang/include/clang/Basic/DiagnosticSemaKinds.td` to improve this. Also we 
need a test for `[[gnu::const]]` too.

https://github.com/llvm/llvm-project/pull/78200
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [coroutines][coro_lifetimebound] Detect lifetime issues with lambda captures (PR #77066)

[Chuanqi Xu via cfe-commits]

@@ -15845,8 +15845,10 @@ void Sema::CheckCoroutineWrapper(FunctionDecl *FD) {
   RecordDecl *RD = FD->getReturnType()->getAsRecordDecl();
   if (!RD || !RD->getUnderlyingDecl()->hasAttr())
 return;
-  // Allow `get_return_object()`.
-  if (FD->getDeclName().isIdentifier() &&
+  // Allow some_promise_type::get_return_object().
+  // Since we are still in the promise definition, we can only do this
+  // heuristically as the promise may not be yet associated to a coroutine.
+  if (isa(FD) && FD->getDeclName().isIdentifier() &&
   FD->getName().equals("get_return_object") && FD->param_empty())
 return;

ChuanqiXu9 wrote:

Got it. In this case, we can make it into a PendingXXX struct in Sema and we 
can check it in the end of the TU. But this is not required in this patch. If 
you don't want to address that in the current patch, let's leave a FIXME or a 
TODO here.

https://github.com/llvm/llvm-project/pull/77066
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [coroutines][coro_lifetimebound] Detect lifetime issues with lambda captures (PR #77066)

[Chuanqi Xu via cfe-commits]

@@ -7575,15 +7577,27 @@ static void 
visitLifetimeBoundArguments(IndirectLocalPath , Expr *Call,
 Path.pop_back();
   };
 
-  if (ObjectArg && implicitObjectParamIsLifetimeBound(Callee))
-VisitLifetimeBoundArg(Callee, ObjectArg);
-
   bool CheckCoroCall = false;
   if (const auto *RD = Callee->getReturnType()->getAsRecordDecl()) {
 CheckCoroCall = RD->hasAttr() &&
 RD->hasAttr() &&
 !Callee->hasAttr();
   }
+
+  if (ObjectArg) {
+bool CheckCoroObjArg = CheckCoroCall;
+// Ignore `__promise.get_return_object()` as it is not lifetimebound.
+if (CheckCoroObjArg && Callee->getDeclName().isIdentifier() &&
+Callee->getName() == "get_return_object")
+  CheckCoroObjArg = false;
+// Coroutine lambda objects with empty capture list are not lifetimebound.
+if (auto *LE = dyn_cast(ObjectArg->IgnoreImplicit());
+LE && LE->captures().empty())
+  CheckCoroObjArg = false;

ChuanqiXu9 wrote:

> > I didn't get the logic here. Why it is not good to warn the undefined 
> > things?
> 
> Sorry, I wasn't clear here. I meant that if it is UB, we should warn for 
> those cases too and remove this code path.
> 
> > I feel it is literally undefined. Since the spec doesn't say a lot about 
> > resumption/suspension about coroutines. Also the description of 
> > http://eel.is/c++draft/dcl.fct.def.coroutine#note-3 is vague too. It says 
> > it is likely to be an undefined behavior.
> 
> Yeah, this looks suspicious. At the same time, it's very similar to `delete 
> this`, which (IIUC) is not UB as long as people are careful to not access 
> `this` after doing it.
> 
> I have actually asked the CWG, but got no reply so far.

Got it. Then it might not be a UB (we didn't access `this` in the empty lambda) 
if we're comparing this case. While I don't object wait for response from CWG, 
I feel we can continue here. On the one hand, we're talking about the behaviors 
with an extension (`[[clang::coro_lifetime_bound]]`) which can be visited as a 
dialect. On the other hand, it is somewhat not user friendly to emit warnings 
in such cases.

https://github.com/llvm/llvm-project/pull/77066
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [AST] Mark the fallthrough coreturn statement implicit. (PR #77465)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 approved this pull request.

LGTM then.

https://github.com/llvm/llvm-project/pull/77465
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [coroutines][coro_lifetimebound] Detect lifetime issues with lambda captures (PR #77066)

[Chuanqi Xu via cfe-commits]

@@ -15845,8 +15845,10 @@ void Sema::CheckCoroutineWrapper(FunctionDecl *FD) {
   RecordDecl *RD = FD->getReturnType()->getAsRecordDecl();
   if (!RD || !RD->getUnderlyingDecl()->hasAttr())
 return;
-  // Allow `get_return_object()`.
-  if (FD->getDeclName().isIdentifier() &&
+  // Allow some_promise_type::get_return_object().
+  // Since we are still in the promise definition, we can only do this
+  // heuristically as the promise may not be yet associated to a coroutine.
+  if (isa(FD) && FD->getDeclName().isIdentifier() &&
   FD->getName().equals("get_return_object") && FD->param_empty())
 return;

ChuanqiXu9 wrote:

My intention here is to remove the special handling to 'get_return_object'. The 
'coro_wrapper'  attribute should be applied here IIUC. 

https://github.com/llvm/llvm-project/pull/77066
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Modules] [HeaderSearch] Don't reenter headers if it is pragma once (PR #76119)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

@vsapsai gentle ping~

https://github.com/llvm/llvm-project/pull/76119
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 7bc170a - [C++20] [Modules] [Serialization] Don't record '#pragma once' information in named modules

[Chuanqi Xu via cfe-commits]

Author: Chuanqi Xu
Date: 2024-01-15T17:00:49+08:00
New Revision: 7bc170a261ae0daaddcc1abeacf7e9e0f1f66d02

URL: 
https://github.com/llvm/llvm-project/commit/7bc170a261ae0daaddcc1abeacf7e9e0f1f66d02
DIFF: 
https://github.com/llvm/llvm-project/commit/7bc170a261ae0daaddcc1abeacf7e9e0f1f66d02.diff

LOG: [C++20] [Modules] [Serialization] Don't record '#pragma once' information 
in named modules

Close https://github.com/llvm/llvm-project/issues/77995

The cause of the issue is that straight forward that we recorded the
'#pragma once' information in named modules, which is an overlook.

I tried to not record the header's information completely within named
modules. But the tests look not happy with some diagnosing problems,
which needs to be looked in details.

Added: 
clang/test/Modules/pr77995.cppm

Modified: 
clang/lib/Serialization/ASTWriter.cpp

Removed: 




diff  --git a/clang/lib/Serialization/ASTWriter.cpp 
b/clang/lib/Serialization/ASTWriter.cpp
index 9950fa9c08faaa..a3fa99c3d7e49f 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -1897,7 +1897,8 @@ namespace {
 
   unsigned char Flags = (Data.AlreadyIncluded << 6)
   | (Data.HFI.isImport << 5)
-  | (Data.HFI.isPragmaOnce << 4)
+  | (Writer.isWritingStdCXXNamedModules() ? 0 :
+ Data.HFI.isPragmaOnce << 4)
   | (Data.HFI.DirInfo << 1)
   | Data.HFI.IndexHeaderMapHeader;
   LE.write(Flags);

diff  --git a/clang/test/Modules/pr77995.cppm b/clang/test/Modules/pr77995.cppm
new file mode 100644
index 00..26442dacac4fb6
--- /dev/null
+++ b/clang/test/Modules/pr77995.cppm
@@ -0,0 +1,25 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/foo.cppm -emit-module-interface -o %t/foo.pcm
+// RUN: %clang_cc1 -std=c++20 %t/use.cpp -fmodule-file=foo=%t/foo.pcm -verify 
-fsyntax-only
+
+//--- a.hpp
+#pragma once
+#define A 43
+
+//--- foo.cppm
+module;
+#include "a.hpp"
+export module foo;
+export constexpr auto B = A;
+
+//--- use.cpp
+// expected-no-diagnostics
+import foo;
+#include "a.hpp"
+
+static_assert(A == 43);
+static_assert(B == 43);
+



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Print library module manifest path. (PR #76451)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 edited 
https://github.com/llvm/llvm-project/pull/76451
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Print library module manifest path. (PR #76451)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 edited 
https://github.com/llvm/llvm-project/pull/76451
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [coroutines][coro_lifetimebound] Detect lifetime issues with lambda captures (PR #77066)

[Chuanqi Xu via cfe-commits]

@@ -33,6 +34,7 @@
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/Support/Casting.h"

ChuanqiXu9 wrote:

Maybe we don't need this?

https://github.com/llvm/llvm-project/pull/77066
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [coroutines][coro_lifetimebound] Detect lifetime issues with lambda captures (PR #77066)

[Chuanqi Xu via cfe-commits]

@@ -15845,8 +15845,10 @@ void Sema::CheckCoroutineWrapper(FunctionDecl *FD) {
   RecordDecl *RD = FD->getReturnType()->getAsRecordDecl();
   if (!RD || !RD->getUnderlyingDecl()->hasAttr())
 return;
-  // Allow `get_return_object()`.
-  if (FD->getDeclName().isIdentifier() &&
+  // Allow some_promise_type::get_return_object().
+  // Since we are still in the promise definition, we can only do this
+  // heuristically as the promise may not be yet associated to a coroutine.
+  if (isa(FD) && FD->getDeclName().isIdentifier() &&
   FD->getName().equals("get_return_object") && FD->param_empty())
 return;

ChuanqiXu9 wrote:

Can we return directly if it has `CoroDisableLifetimeBoundAttr`? (Maybe we need 
to edit the semantics of `CoroDisableLifetimeBoundAttr` a little bit)

https://github.com/llvm/llvm-project/pull/77066
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits