Skip to content

Conversation

EricWF
Copy link
Member

@EricWF EricWF commented Aug 2, 2025

No description provided.

EricWF added 30 commits June 19, 2024 14:09
The template stuff is a bit of a hack. I'm considering simply inserting
the statements into the function body and letting the existing code
handle the rest.

Also add some cheaky diagnostics, fix a few name lookup bugs, and
delete some old code.
There are also various experiements that need cleaning up,
specifically around builtins and source location.
Some work on constification, but a bunch of experiment that still needs
to be undone.

The addition of -fclang-contract-groups=foo=<semantic>, which still
needs better integrating with -fcontract-evaluation-semantic=.

The [[clang::contract_group("foo.bar")]] stuff works really well.
I'll be happy to try it in libc++.

What still needs to be done includes:
  * Coming up with a proper descriptor/format for the compiler-stl
    interface. It seems to me we'll want the ABI to be able to evolve
over time, and so we should pass a pointer to data + data descriptor of
some kind so that the STL can parse version of the header from different
compilers & versions.

  * Everything to do with parsing pre/post in inline member functions.
  * Contract redeclaration checking.
  * Constifying implicit this.
  * Code gen & constant evaluation for result names (and representation
    too).
  * Cleanup around experiments for source location & constification.
The template stuff is a bit of a hack. I'm considering simply inserting
the statements into the function body and letting the existing code
handle the rest.

Also add some cheaky diagnostics, fix a few name lookup bugs, and
delete some old code.
There are also various experiements that need cleaning up,
specifically around builtins and source location.
Some work on constification, but a bunch of experiment that still needs
to be undone.

The addition of -fclang-contract-groups=foo=<semantic>, which still
needs better integrating with -fcontract-evaluation-semantic=.

The [[clang::contract_group("foo.bar")]] stuff works really well.
I'll be happy to try it in libc++.

What still needs to be done includes:
  * Coming up with a proper descriptor/format for the compiler-stl
    interface. It seems to me we'll want the ABI to be able to evolve
over time, and so we should pass a pointer to data + data descriptor of
some kind so that the STL can parse version of the header from different
compilers & versions.

  * Everything to do with parsing pre/post in inline member functions.
  * Contract redeclaration checking.
  * Constifying implicit this.
  * Code gen & constant evaluation for result names (and representation
    too).
  * Cleanup around experiments for source location & constification.
This allows better versioning, and also prevents us from clobbering
every registery. (though that latter case is likely not too impactful).
Also, re-enable the exception handling tentatively.
Copy link

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am far from having done a complete pass, but here are some comments.
Chopping this in manageable pieces will be necessary I'm afraid, Github can barely render the PR and reviewers will be discouraged by the sheer size of it all.

Changes to source location could be a PR and then maybe we want to start with just contract_assert, even if i understand it would be annoying for you.

Non essentials (ast matchers for examples) can be separate PRs.
Everything lamba related can also be separated out.

I will keep reviewing today or tomorrow

@@ -1287,6 +1287,10 @@ class ASTContext : public RefCountedBase<ASTContext> {
// The decl is built when constructing 'BuiltinVaListDecl'.
mutable Decl *VaListTagDecl = nullptr;

// Decl used to define the datastructure for the contract violation object
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Decl used to define the datastructure for the contract violation object
// Decl used to define the data structure for the contract violation object

@@ -1287,6 +1287,10 @@ class ASTContext : public RefCountedBase<ASTContext> {
// The decl is built when constructing 'BuiltinVaListDecl'.
mutable Decl *VaListTagDecl = nullptr;

// Decl used to define the datastructure for the contract violation object
// used for C++ contracts
mutable Decl *BuiltinContractViolationRecordDecl = nullptr;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to make that a RecordDecl directly?

Comment on lines 161 to +162
if (isa<DeclStmt, GenericSelectionExpr, RequiresExpr,
OpenACCWaitConstruct, SYCLKernelCallStmt>(S))
OpenACCWaitConstruct, SYCLKernelCallStmt, ContractStmt>(S))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should just make expressions children. It's not yet clear to me why they should not be, as I do a linear read through

/// int foo() post(r : r > 0);
///
/// Where `r` refers to the value returned by the function
class ResultNameDecl : public ValueDecl {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be called ContractXXXX. Maybe ContractNamedReturnDecl

// qualifier, to be used for the (uncommon) case of out-of-line declarations
// and constrained function decls.
// qualifier, to be used for the (uncommon) case of out-of-line declarations,
// constrained function decls or functions with contracts.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// constrained function decls or functions with contracts.
// constrained function decls or functions with pre/post conditions.

@@ -287,6 +290,7 @@ PUNCTUATOR(greatergreatergreater, ">>>")
// which are heavily based on AltiVec
// KEYBORLAND - This is a keyword if Borland extensions are enabled
// KEYCOROUTINES - This is a keyword if support for C++ coroutines is enabled
// KEYCONTRACTS - This is a keyword if support for C++ contracts is enabled
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the upstream version I think

  • we want to put contracts behind the C++26 flags
  • allow them in all c++ versions (without flag) once they are a bit baked

Comment on lines +1630 to +1636
// C++ Contracts
defm contracts : BoolFOption<"contracts",
LangOpts<"Contracts">, DefaultFalse,
PosFlag<SetTrue, [], [ClangOption, CC1Option, CLOption],
"Enable support for C++ Contracts">,
NegFlag<SetFalse, [], [ClangOption, CC1Option]>>;

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cf above, lets forgo that for now

Comment on lines +1646 to +1656
def fcontract_group_evaluation_semantic_EQ : CommaJoined<["-"], "fcontract-group-evaluation-semantic=">,
Visibility<[ClangOption, CC1Option]>,
HelpText<"Clang extension. Enable or disable contracts by group. The argument is a comma-separated "
"sequence of one or more group names, each prefixed by '+' or '-'.">;

defm contract_exceptions : BoolFOption<"contract-exceptions",
LangOpts<"ContractExceptions">, DefaultTrue,
PosFlag<SetTrue, [], [ClangOption],
"Enable contract violation on exception">,
NegFlag<SetFalse, [], [ClangOption, CC1Option],
"Disable contract violations on exception">>;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do that in a subsequent PR?

Comment on lines +8958 to +8972
std::optional<ContractKind> getContractKeyword(const Token &Token) const;
std::optional<ContractKind> getContractKeyword() const {
return getContractKeyword(Tok);
}
bool isFunctionContractKeyword() const {
return isFunctionContractKeyword(Tok);
}
bool isFunctionContractKeyword(const Token &Token) const {
return getContractKeyword(Token).value_or(ContractKind::Assert) !=
ContractKind::Assert;
}

bool isAnyContractKeyword(const Token &Token) const {
return getContractKeyword(Token).has_value();
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cf above, lets keep assert separate from pre/post

@@ -42,7 +43,7 @@ class Scope {
public:
/// ScopeFlags - These are bitfields that are or'd together when creating a
/// scope, which defines the sorts of things the scope contains.
enum ScopeFlags {
enum ScopeFlags : unsigned long {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we want to introduce an alias for the underlying type

Comment on lines +110 to +116
enum class ContractEmissionStyle {
/// Emit the contract at every call site, in the caller.
InCaller,

/// Emit the contract with the function definition.
InDefinition,
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we focus on callee side initially?

Comment on lines +121 to +125
enum ContractScopeOffset {
CSO_ParentContext,
CSO_FunctionContext
};

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets use a scoped enum here

"woops... FIXME(EricWF): %0"
>, InGroup<EricWFFixme>;

def err_result_name_shadows_param : Error<
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we be more consistent about using _contract_ in these diags?

def err_result_name_shadows_param : Error<
"declaration of result name %0 shadows parameter">;
def err_void_result_name : Error<
"result name %0 cannot be used with a void return type">;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

used? declared? introduced?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"result name in the post condition of a function with a void return type" maybe

Comment on lines +12595 to +12603
def err_contract_violation : Error<"contract %select{precondition|postcondition|assertion}0 failed%select{: %2|}1">;
def warn_contract_violation : Warning<"contract %select{precondition|postcondition|assertion}0 failed%select{: %2|}1">,
InGroup<ContractViolation>;

def err_contract_requirement_failed : Error<
"%select{precondition|postcondition|contract assertion}0 failed due to requirement '%1'%select{: %3|}2">;
def warn_contract_requirement_failed : Warning<
"%select{precondition|postcondition|contract assertion}0 failed due to requirement '%1'%select{: %3|}2">,
InGroup<ContractViolation>;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of these are (nor can) ever be used, right?

Comment on lines +290 to +291
ContractSpecifierDecl *
Parser::ParseLexedFunctionContractsInScope(CachedTokens &ContractToks,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that used?

Comment on lines +190 to +192
ContractSpecifierDecl *Seq = Actions.ActOnFinishContractSpecifierSequence(
Contracts, StartLoc, IsInvalid);

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should check we are parsing a function (and not a type or a function pointer)
see err_requires_clause_on_declarator_not_declaring_a_function

Comment on lines +238 to +240
// FIXME(EricWF): We allow parsing the result name declarator in `pre` so we
// can diagnose it but we don't do the same for contract assert... Should we?
if ((CK != ContractKind::Assert) && Tok.is(tok::identifier) &&
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably should not!

Comment on lines +249 to +251
if (ReturnTypeResolver) {
ReturnType = ReturnTypeResolver();
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (ReturnTypeResolver) {
ReturnType = ReturnTypeResolver();
}
if (ReturnTypeResolver)
ReturnType = ReturnTypeResolver();

just because i noticed :)

Comment on lines +274 to +280
if (Cond.isInvalid()) {
Cond =
Actions.CreateRecoveryExpr(ExprLoc, EndLoc, {}, Actions.Context.BoolTy);
} else {
SetInvalidOnExit.release();
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here for the braces

EricWF pushed a commit that referenced this pull request Aug 7, 2025
…erver (llvm#148774)

Summary:
There was a deadlock was introduced by [PR
llvm#146441](llvm#146441) which changed
`CurrentThreadIsPrivateStateThread()` to
`CurrentThreadPosesAsPrivateStateThread()`. This change caused the
execution path in
[`ExecutionContextRef::SetTargetPtr()`](https://github.com/llvm/llvm-project/blob/10b5558b61baab59c7d3dff37ffdf0861c0cc67a/lldb/source/Target/ExecutionContext.cpp#L513)
to now enter a code block that was previously skipped, triggering
[`GetSelectedFrame()`](https://github.com/llvm/llvm-project/blob/10b5558b61baab59c7d3dff37ffdf0861c0cc67a/lldb/source/Target/ExecutionContext.cpp#L522)
which leads to a deadlock.

Thread 1 gets m_modules_mutex in
[`ModuleList::AppendImpl`](https://github.com/llvm/llvm-project/blob/96148f92146e5211685246722664e51ec730e7ba/lldb/source/Core/ModuleList.cpp#L218),
Thread 3 gets m_language_runtimes_mutex in
[`GetLanguageRuntime`](https://github.com/llvm/llvm-project/blob/96148f92146e5211685246722664e51ec730e7ba/lldb/source/Target/Process.cpp#L1501),
but then Thread 1 waits for m_language_runtimes_mutex in
[`GetLanguageRuntime`](https://github.com/llvm/llvm-project/blob/96148f92146e5211685246722664e51ec730e7ba/lldb/source/Target/Process.cpp#L1501)
while Thread 3 waits for m_modules_mutex in
[`ScanForGNUstepObjCLibraryCandidate`](https://github.com/llvm/llvm-project/blob/96148f92146e5211685246722664e51ec730e7ba/lldb/source/Plugins/LanguageRuntime/ObjC/GNUstepObjCRuntime/GNUstepObjCRuntime.cpp#L57).

This fixes the deadlock by adding a scoped block around the mutex lock
before the call to the notifier, and moved the notifier call outside of
the mutex-guarded section. The notifier call
[`NotifyModuleAdded`](https://github.com/llvm/llvm-project/blob/96148f92146e5211685246722664e51ec730e7ba/lldb/source/Target/Target.cpp#L1810)
should be thread-safe, since the module should be added to the
`ModuleList` before the mutex is released, and the notifier doesn't
modify the module list further, and the call is operates on local state
and the `Target` instance.

### Deadlocked Thread backtraces:
```
* thread #3, name = 'dbg.evt-handler', stop reason = signal SIGSTOP
  * frame #0: 0x00007f2f1e2973dc libc.so.6`futex_wait(private=0, expected=2, futex_word=0x0000563786bd5f40) at    futex-internal.h:146:13
   /*... a bunch of mutex related bt ... */    
   liblldb.so.21.0git`std::lock_guard<std::recursive_mutex>::lock_guard(this=0x00007f2f0f1927b0, __m=0x0000563786bd5f40) at std_mutex.h:229:19
    frame #8: 0x00007f2f27946eb7 liblldb.so.21.0git`ScanForGNUstepObjCLibraryCandidate(modules=0x0000563786bd5f28, TT=0x0000563786bd5eb8) at GNUstepObjCRuntime.cpp:60:41
    frame #9: 0x00007f2f27946c80 liblldb.so.21.0git`lldb_private::GNUstepObjCRuntime::CreateInstance(process=0x0000563785e1d360, language=eLanguageTypeObjC) at GNUstepObjCRuntime.cpp:87:8
    frame llvm#10: 0x00007f2f2746fca5 liblldb.so.21.0git`lldb_private::LanguageRuntime::FindPlugin(process=0x0000563785e1d360, language=eLanguageTypeObjC) at LanguageRuntime.cpp:210:36
    frame llvm#11: 0x00007f2f2742c9e3 liblldb.so.21.0git`lldb_private::Process::GetLanguageRuntime(this=0x0000563785e1d360, language=eLanguageTypeObjC) at Process.cpp:1516:9
    ...
    frame llvm#21: 0x00007f2f2750b5cc liblldb.so.21.0git`lldb_private::Thread::GetSelectedFrame(this=0x0000563785e064d0, select_most_relevant=DoNoSelectMostRelevantFrame) at Thread.cpp:274:48
    frame llvm#22: 0x00007f2f273f9957 liblldb.so.21.0git`lldb_private::ExecutionContextRef::SetTargetPtr(this=0x00007f2f0f193778, target=0x0000563786bd5be0, adopt_selected=true) at ExecutionContext.cpp:525:32
    frame llvm#23: 0x00007f2f273f9714 liblldb.so.21.0git`lldb_private::ExecutionContextRef::ExecutionContextRef(this=0x00007f2f0f193778, target=0x0000563786bd5be0, adopt_selected=true) at ExecutionContext.cpp:413:3
    frame llvm#24: 0x00007f2f270e80af liblldb.so.21.0git`lldb_private::Debugger::GetSelectedExecutionContext(this=0x0000563785d83bc0) at Debugger.cpp:1225:23
    frame llvm#25: 0x00007f2f271bb7fd liblldb.so.21.0git`lldb_private::Statusline::Redraw(this=0x0000563785d83f30, update=true) at Statusline.cpp:136:41
    ...
* thread #1, name = 'lldb', stop reason = signal SIGSTOP
  * frame #0: 0x00007f2f1e2973dc libc.so.6`futex_wait(private=0, expected=2, futex_word=0x0000563785e1dd98) at futex-internal.h:146:13
   /*... a bunch of mutex related bt ... */    
   liblldb.so.21.0git`std::lock_guard<std::recursive_mutex>::lock_guard(this=0x00007ffe62be0488, __m=0x0000563785e1dd98) at std_mutex.h:229:19
    frame #8: 0x00007f2f2742c8d1 liblldb.so.21.0git`lldb_private::Process::GetLanguageRuntime(this=0x0000563785e1d360, language=eLanguageTypeC_plus_plus) at Process.cpp:1510:41
    frame #9: 0x00007f2f2743c46f liblldb.so.21.0git`lldb_private::Process::ModulesDidLoad(this=0x0000563785e1d360, module_list=0x00007ffe62be06a0) at Process.cpp:6082:36
    ...
    frame llvm#13: 0x00007f2f2715cf03 liblldb.so.21.0git`lldb_private::ModuleList::AppendImpl(this=0x0000563786bd5f28, module_sp=ptr = 0x563785cec560, use_notifier=true) at ModuleList.cpp:246:19
    frame llvm#14: 0x00007f2f2715cf4c liblldb.so.21.0git`lldb_private::ModuleList::Append(this=0x0000563786bd5f28, module_sp=ptr = 0x563785cec560, notify=true) at ModuleList.cpp:251:3
    ...
    frame llvm#19: 0x00007f2f274349b3 liblldb.so.21.0git`lldb_private::Process::ConnectRemote(this=0x0000563785e1d360, remote_url=(Data = "connect://localhost:1234", Length = 24)) at Process.cpp:3250:9
    frame llvm#20: 0x00007f2f27411e0e liblldb.so.21.0git`lldb_private::Platform::DoConnectProcess(this=0x0000563785c59990, connect_url=(Data = "connect://localhost:1234", Length = 24), plugin_name=(Data = "gdb-remote", Length = 10), debugger=0x0000563785d83bc0, stream=0x00007ffe62be3128, target=0x0000563786bd5be0, error=0x00007ffe62be1ca0) at Platform.cpp:1926:23
```

## Test Plan:
Built a hello world a.out
Run server in one terminal:
```
~/llvm/build/Debug/bin/lldb-server g :1234 a.out
```
Run client in another terminal
```
~/llvm/build/Debug/bin/lldb -o "gdb-remote 1234" -o "b hello.cc:3"
```

Before:
Client hangs indefinitely
```
~/llvm/build/Debug/bin/lldb -o "gdb-remote 1234" -o "b main"
(lldb) gdb-remote 1234

^C^C
```

After:
```
~/llvm/build/Debug/bin/lldb -o "gdb-remote 1234" -o "b hello.cc:3"
(lldb) gdb-remote 1234
Process 837068 stopped
* thread #1, name = 'a.out', stop reason = signal SIGSTOP
    frame #0: 0x00007ffff7fe4a60
ld-linux-x86-64.so.2`_start:
->  0x7ffff7fe4a60 <+0>: movq   %rsp, %rdi
    0x7ffff7fe4a63 <+3>: callq  0x7ffff7fe5780 ; _dl_start at rtld.c:522:1

ld-linux-x86-64.so.2`_dl_start_user:
    0x7ffff7fe4a68 <+0>: movq   %rax, %r12
    0x7ffff7fe4a6b <+3>: movl   0x18067(%rip), %eax ; _dl_skip_args
(lldb) b hello.cc:3
Breakpoint 1: where = a.out`main + 15 at hello.cc:4:13, address = 0x00005555555551bf
(lldb) c
Process 837068 resuming
Process 837068 stopped
* thread #1, name = 'a.out', stop reason = breakpoint 1.1
    frame #0: 0x00005555555551bf a.out`main at hello.cc:4:13
   1   	#include <iostream>
   2
   3   	int main() {
-> 4   	  std::cout << "Hello World" << std::endl;
   5   	  return 0;
   6   	}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants