[Lldb-commits] [PATCH] D144688: [lldb] Fix watchpoint command function stopping behavior

[Dan Liew via Phabricator via lldb-commits]

delcypher added inline comments.



Comment at: lldb/source/Commands/CommandObjectWatchpointCommand.cpp:425
   else if (!m_options.m_function_name.empty()) {
-std::string oneliner(m_options.m_function_name);
+std::string oneliner("return ");
+oneliner += m_options.m_function_name;

@mib There's a reason I didn't do it this way when I tried to fix this locally.

The python stub function we generate would look something like this with your 
patch.

```lang=python
def lldb_autogen_python_wp_callback_func__0 (frame, wp, internal_dict):
 global_dict = globals()
 new_keys = internal_dict.keys()
 old_keys = global_dict.keys()
 global_dict.update (internal_dict)
 if True:
 return call_user_function(frame, wp, internal_dict)
 for key in new_keys:
 internal_dict[key] = global_dict[key]
 if key not in old_keys:
del global_dict[key]
```

with your early return the logic for updating `internal_dict` and `global_dict` 
will **not run**.  I'm not entirely sure what the purpose of this is but if its 
important then we need to implement this patch another way.

Here's another way we could do it. You could take the patch I originally wrote 
but change `ScriptInterpreterPythonImpl::GenerateFunction(...)` to take an 
additional parameter `bool ReturnValue` that is false by default. This 
parameter when `true` would do the work my patch did to make sure we use the 
return value from the last user statement evaluated. For the watchpoint 
evaluation we can pass a `true` value. For the other cases `ReturnValue` will 
be false so there will be no behavior change there.



Comment at: 
lldb/test/API/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandPython.py:159
+
+self.expect("thread backtrace", STOPPED_DUE_TO_BREAKPOINT,
+substrs=['stop reason = breakpoint'])

This `self.expect()` fails when the bug hasn't been fixed, correct?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144688

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

[Lldb-commits] [PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

[Aaron Ballman via Phabricator via lldb-commits]

aaron.ballman edited reviewers, added: erichkeane, royjacobson, 
clang-language-wg; removed: mizvekov.
aaron.ballman added a subscriber: rjmccall.
aaron.ballman added a comment.

In D140996#4125177 , @bolshakov-a 
wrote:

> @aaron.ballman, @rsmith, @mizvekov, @shafik,  has the mankind any chance to 
> get this reviewed and merged?

Sorry for the delay in review! I've changed the reviewer list a bit to get more 
visibility on this. Also, don't forget to add a release note for the changes. 
Should this also update the status in `clang/www/cxx_status.html`?




Comment at: clang/lib/AST/ItaniumMangle.cpp:4397
+// argument.
+// As proposed in https://github.com/itanium-cxx-abi/cxx-abi/issues/111.
+auto *SNTTPE = cast(E);

We should get this nailed down. It was proposed in Nov 2020 and the issue is 
still open. CC @rjmccall 



Comment at: clang/lib/AST/TemplateBase.cpp:204-211
+  if (Type->isIntegralOrEnumerationType() && V.isInt())
+*this = TemplateArgument(Ctx, V.getInt(), Type);
+  else if ((V.isLValue() && V.isNullPointer()) ||
+   (V.isMemberPointer() && !V.getMemberPointerDecl()))
+*this = TemplateArgument(Type, /*isNullPtr=*/true);
+  else if (const ValueDecl *VD = getAsSimpleValueDeclRef(Ctx, Type, V))
+// FIXME: The Declaration form should expose a const ValueDecl*.

Well this is certainly a unique approach...  Personally, I think it'd be nice 
to not assign to `this` within a constructor by calling other constructor; that 
falls squarely into "wait, what, can you even DO that?" kind of questions for 
me.



Comment at: clang/lib/AST/TemplateBase.cpp:619
+  case TemplateArgument::UncommonValue: {
+// FIXME: We're guessing at LangOptions!
+SmallString<32> Str;

It's probably okay enough, but this is now the third instance of adding the 
same bug to this helper method -- maybe we should fix that instead?



Comment at: clang/lib/Index/USRGeneration.cpp:1032
+  case TemplateArgument::UncommonValue:
+// FIXME: Visit value.
+break;

Any particular reason this isn't being handled now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140996

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

[Lldb-commits] [PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

[Erich Keane via Phabricator via lldb-commits]

erichkeane added inline comments.



Comment at: clang/include/clang/AST/ASTContext.h:3036
 if (!std::is_trivially_destructible::value) {
-  auto DestroyPtr = [](void *V) { static_cast(V)->~T(); };
-  AddDeallocation(DestroyPtr, Ptr);
+  auto DestroyPtr = [](void *V) { ((T *)V)->~T(); };
+  AddDeallocation(DestroyPtr, (void *)Ptr);

This change is weird... what is going on here?



Comment at: clang/include/clang/AST/TemplateBase.h:88
+/// so cannot be dependent.
+UncommonValue,
+

I definitely hate the name here... Just `Value` makes a bit more sense, but 
isn't perfectly accurate.  Perhaps `NonTypeValue`?  But that is a little 
redundant.  `Uncommon` here is just strange and not particularly descriptive. 



Comment at: clang/lib/AST/MicrosoftMangle.cpp:1670
+  case TemplateArgument::UncommonValue:
+if (ValueDecl *D = getAsArrayToPointerDecayedDecl(
+TA.getUncommonValueType(), TA.getAsUncommonValue())) {

Has microsoft implemented this yet?  Can we see what they chose to do and make 
sure we match them? 



Comment at: clang/lib/AST/ODRHash.cpp:177
+case TemplateArgument::UncommonValue:
+  // FIXME: Include a representation of these arguments.
   break;

I feel like this DEFINITELY needs to happen.  Nullptr/integral is less 
important, but now that we have an actual value here, I think it needs to be 
part of the hash.



Comment at: clang/lib/AST/TemplateBase.cpp:204-211
+  if (Type->isIntegralOrEnumerationType() && V.isInt())
+*this = TemplateArgument(Ctx, V.getInt(), Type);
+  else if ((V.isLValue() && V.isNullPointer()) ||
+   (V.isMemberPointer() && !V.getMemberPointerDecl()))
+*this = TemplateArgument(Type, /*isNullPtr=*/true);
+  else if (const ValueDecl *VD = getAsSimpleValueDeclRef(Ctx, Type, V))
+// FIXME: The Declaration form should expose a const ValueDecl*.

aaron.ballman wrote:
> Well this is certainly a unique approach...  Personally, I think it'd be nice 
> to not assign to `this` within a constructor by calling other constructor; 
> that falls squarely into "wait, what, can you even DO that?" kind of 
> questions for me.
I agree, this function needs to be refactored to call some sort of 'init' 
function or something, even if we have to refactor the other constructors.  
This assignment to `*this` is just too strange to let stay.



Comment at: clang/lib/AST/TemplateBase.cpp:619
+  case TemplateArgument::UncommonValue: {
+// FIXME: We're guessing at LangOptions!
+SmallString<32> Str;

aaron.ballman wrote:
> It's probably okay enough, but this is now the third instance of adding the 
> same bug to this helper method -- maybe we should fix that instead?
Agreed, seems to me we should do the work NOW to just wire in the lang-opts to 
this function.



Comment at: clang/lib/Sema/SemaTemplate.cpp:7882
+Sema &S, QualType T, const APValue &Val, SourceLocation Loc) {
+  auto MakeInitList = [&](ArrayRef Elts) -> Expr * {
+auto *ILE = new (S.Context) InitListExpr(S.Context, Loc, Elts, Loc);

I don't have a good idea of what is happening in this function here, it isn't 
really clear... before committing, someone needs to do a deep dive on this 
function for review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140996

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

[Lldb-commits] [PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

[Andrey Ali Khan Bolshakov via Phabricator via lldb-commits]

bolshakov-a added a subscriber: mizvekov.
bolshakov-a added a comment.

Thanks for the review! I definitely can try to answer questions and fix issues, 
but I just want to note that I'm not the original author, and these changes 
already were upstream. (Maybe, @rsmith will find some time for taking a look at 
this, after all?)

It would be nice if @mizvekov would check my changes regarding 
`SugaredConverted` vs `CanonicalConverted`.

> Should this also update the status in clang/www/cxx_status.html?

I'm not sure. There still remains an issue with template parameters of 
reference type, which was tried to be fixed in 5a391d38ac6c 
. (On the 
other hand, MSVC team claims that their compiler supports P1907R1, despite that 
it simply crashes on some of the test cases from 5a391d38ac6c 
)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140996

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

[Lldb-commits] [PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

[Aaron Ballman via Phabricator via lldb-commits]

aaron.ballman added a comment.

In D140996#4150848 , @bolshakov-a 
wrote:

>> Should this also update the status in clang/www/cxx_status.html?
>
> I'm not sure. There still remains an issue with template parameters of 
> reference type, which was tried to be fixed in 5a391d38ac6c 
> . (On 
> the other hand, MSVC team claims that their compiler supports P1907R1, 
> despite that it simply crashes on some of the test cases from 5a391d38ac6c 
> )

Hmmm, okay, then I'd say we probably should update cxx_status. We can leave the 
status as "partial" but we can at least add details about what's still missing 
before we can claim full support. Kind of like we do for P0848R3 or P0315R4 on 
that page.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140996

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

[Lldb-commits] [PATCH] D144688: [lldb] Fix watchpoint command function stopping behavior

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a subscriber: jingham.
mib added inline comments.



Comment at: lldb/source/Commands/CommandObjectWatchpointCommand.cpp:425
   else if (!m_options.m_function_name.empty()) {
-std::string oneliner(m_options.m_function_name);
+std::string oneliner("return ");
+oneliner += m_options.m_function_name;

delcypher wrote:
> @mib There's a reason I didn't do it this way when I tried to fix this 
> locally.
> 
> The python stub function we generate would look something like this with your 
> patch.
> 
> ```lang=python
> def lldb_autogen_python_wp_callback_func__0 (frame, wp, internal_dict):
>  global_dict = globals()
>  new_keys = internal_dict.keys()
>  old_keys = global_dict.keys()
>  global_dict.update (internal_dict)
>  if True:
>  return call_user_function(frame, wp, internal_dict)
>  for key in new_keys:
>  internal_dict[key] = global_dict[key]
>  if key not in old_keys:
> del global_dict[key]
> ```
> 
> with your early return the logic for updating `internal_dict` and 
> `global_dict` will **not run**.  I'm not entirely sure what the purpose of 
> this is but if its important then we need to implement this patch another way.
> 
> Here's another way we could do it. You could take the patch I originally 
> wrote but change `ScriptInterpreterPythonImpl::GenerateFunction(...)` to take 
> an additional parameter `bool ReturnValue` that is false by default. This 
> parameter when `true` would do the work my patch did to make sure we use the 
> return value from the last user statement evaluated. For the watchpoint 
> evaluation we can pass a `true` value. For the other cases `ReturnValue` will 
> be false so there will be no behavior change there.
It's funny you're proposing this approach because we had the exact same idea 
when looking at this bug with @bulbazord.

I ended up going with this implementation because if you use and one-liner `-o` 
instead of a python function `-F` for your watchpoint callback, you'd also get 
the early return behavior.

I thought it would be better to stay consistant even if that implies leaving 
some `internal_dict` keys in the `global_dict` (because of the early return).

 May be @jingham has some opinions about this.



Comment at: 
lldb/test/API/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandPython.py:159
+
+self.expect("thread backtrace", STOPPED_DUE_TO_BREAKPOINT,
+substrs=['stop reason = breakpoint'])

delcypher wrote:
> This `self.expect()` fails when the bug hasn't been fixed, correct?
Yup


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144688

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

[Lldb-commits] [PATCH] D144688: [lldb] Fix watchpoint command function stopping behavior

[Alex Langford via Phabricator via lldb-commits]

bulbazord accepted this revision.
bulbazord added a comment.
This revision is now accepted and ready to land.

I'm alright with this patch since it at least makes watchpoint commands 
consistent with breakpoint commands (w.r.t the `-F` flag).




Comment at: lldb/source/Commands/CommandObjectWatchpointCommand.cpp:425
   else if (!m_options.m_function_name.empty()) {
-std::string oneliner(m_options.m_function_name);
+std::string oneliner("return ");
+oneliner += m_options.m_function_name;

mib wrote:
> delcypher wrote:
> > @mib There's a reason I didn't do it this way when I tried to fix this 
> > locally.
> > 
> > The python stub function we generate would look something like this with 
> > your patch.
> > 
> > ```lang=python
> > def lldb_autogen_python_wp_callback_func__0 (frame, wp, internal_dict):
> >  global_dict = globals()
> >  new_keys = internal_dict.keys()
> >  old_keys = global_dict.keys()
> >  global_dict.update (internal_dict)
> >  if True:
> >  return call_user_function(frame, wp, internal_dict)
> >  for key in new_keys:
> >  internal_dict[key] = global_dict[key]
> >  if key not in old_keys:
> > del global_dict[key]
> > ```
> > 
> > with your early return the logic for updating `internal_dict` and 
> > `global_dict` will **not run**.  I'm not entirely sure what the purpose of 
> > this is but if its important then we need to implement this patch another 
> > way.
> > 
> > Here's another way we could do it. You could take the patch I originally 
> > wrote but change `ScriptInterpreterPythonImpl::GenerateFunction(...)` to 
> > take an additional parameter `bool ReturnValue` that is false by default. 
> > This parameter when `true` would do the work my patch did to make sure we 
> > use the return value from the last user statement evaluated. For the 
> > watchpoint evaluation we can pass a `true` value. For the other cases 
> > `ReturnValue` will be false so there will be no behavior change there.
> It's funny you're proposing this approach because we had the exact same idea 
> when looking at this bug with @bulbazord.
> 
> I ended up going with this implementation because if you use and one-liner 
> `-o` instead of a python function `-F` for your watchpoint callback, you'd 
> also get the early return behavior.
> 
> I thought it would be better to stay consistant even if that implies leaving 
> some `internal_dict` keys in the `global_dict` (because of the early return).
> 
>  May be @jingham has some opinions about this.
The behavior implemented in this patch makes the `-F` flag consistent with the 
way it behaves with breakpoint commands. If we want to make sure that 
`internal_dict` and `global_dict` are appropriately updated instead of doing an 
early return, then I think that should be done in a follow-up that would apply 
to both breakpoints and watchpoints (because they shouldn't really be behaving 
differently w.r.t. commands). A simple idea for that would be to store the 
result of the function call in a variable and return it at the very end.



Comment at: 
lldb/test/API/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandPython.py:1
-"""
+"""
 Test 'watchpoint command'.

I checked, the difference here is a byte order mark: 
https://en.wikipedia.org/wiki/Byte_order_mark

This just removes 3 bytes: ef bb bf


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144688

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

[Lldb-commits] [PATCH] D144688: [lldb] Fix watchpoint command function stopping behavior

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.



Comment at: lldb/source/Commands/CommandObjectWatchpointCommand.cpp:425
   else if (!m_options.m_function_name.empty()) {
-std::string oneliner(m_options.m_function_name);
+std::string oneliner("return ");
+oneliner += m_options.m_function_name;

bulbazord wrote:
> mib wrote:
> > delcypher wrote:
> > > @mib There's a reason I didn't do it this way when I tried to fix this 
> > > locally.
> > > 
> > > The python stub function we generate would look something like this with 
> > > your patch.
> > > 
> > > ```lang=python
> > > def lldb_autogen_python_wp_callback_func__0 (frame, wp, internal_dict):
> > >  global_dict = globals()
> > >  new_keys = internal_dict.keys()
> > >  old_keys = global_dict.keys()
> > >  global_dict.update (internal_dict)
> > >  if True:
> > >  return call_user_function(frame, wp, internal_dict)
> > >  for key in new_keys:
> > >  internal_dict[key] = global_dict[key]
> > >  if key not in old_keys:
> > > del global_dict[key]
> > > ```
> > > 
> > > with your early return the logic for updating `internal_dict` and 
> > > `global_dict` will **not run**.  I'm not entirely sure what the purpose 
> > > of this is but if its important then we need to implement this patch 
> > > another way.
> > > 
> > > Here's another way we could do it. You could take the patch I originally 
> > > wrote but change `ScriptInterpreterPythonImpl::GenerateFunction(...)` to 
> > > take an additional parameter `bool ReturnValue` that is false by default. 
> > > This parameter when `true` would do the work my patch did to make sure we 
> > > use the return value from the last user statement evaluated. For the 
> > > watchpoint evaluation we can pass a `true` value. For the other cases 
> > > `ReturnValue` will be false so there will be no behavior change there.
> > It's funny you're proposing this approach because we had the exact same 
> > idea when looking at this bug with @bulbazord.
> > 
> > I ended up going with this implementation because if you use and one-liner 
> > `-o` instead of a python function `-F` for your watchpoint callback, you'd 
> > also get the early return behavior.
> > 
> > I thought it would be better to stay consistant even if that implies 
> > leaving some `internal_dict` keys in the `global_dict` (because of the 
> > early return).
> > 
> >  May be @jingham has some opinions about this.
> The behavior implemented in this patch makes the `-F` flag consistent with 
> the way it behaves with breakpoint commands. If we want to make sure that 
> `internal_dict` and `global_dict` are appropriately updated instead of doing 
> an early return, then I think that should be done in a follow-up that would 
> apply to both breakpoints and watchpoints (because they shouldn't really be 
> behaving differently w.r.t. commands). A simple idea for that would be to 
> store the result of the function call in a variable and return it at the very 
> end.
+1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144688

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

[Lldb-commits] [PATCH] D144688: [lldb] Fix watchpoint command function stopping behavior

[Jim Ingham via Phabricator via lldb-commits]

jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.

IIUC, not removing elements from the internal dict will mean 
cross-contamination between the python states when you have multiple concurrent 
debuggers, which for instance Xcode does a lot.  New variables would appear in 
the other session, but not linked to this debugger.  That could cause subtle 
problems, which it's worth doing some extra work to avoid.

We should also make sure in the test that a watchpoint command that returns 
nothing still works.  That's suppose to be the equivalent of returning True.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144688

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

[Lldb-commits] [PATCH] D144688: [lldb] Fix watchpoint command function stopping behavior

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

In D144688#4151164 , @jingham wrote:

> IIUC, not removing elements from the internal dict will mean 
> cross-contamination between the python states when you have multiple 
> concurrent debuggers, which for instance Xcode does a lot.  New variables 
> would appear in the other session, but not linked to this debugger.  That 
> could cause subtle problems, which it's worth doing some extra work to avoid.

We've discuss offline with @jingham, and I'll fix this on a new diff to 
separate the 2 issues.

> We should also make sure in the test that a watchpoint command that returns 
> nothing still works.  That's suppose to be the equivalent of returning True.

I'll update this PR to test this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144688

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

[Lldb-commits] [lldb] 1c417da - Remove uses of ATOMIC_VAR_INIT

[Fangrui Song via lldb-commits]

Author: Fangrui Song
Date: 2023-02-24T13:43:12-08:00
New Revision: 1c417da0f0605e027eb02fb4108d08397b566c3b

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

LOG: Remove uses of ATOMIC_VAR_INIT

ATOMIC_VAR_INIT has a trivial definition `#define ATOMIC_VAR_INIT(value) 
(value)`,
is deprecated in C17/C++20, and will be removed in newer standards.

Added: 


Modified: 
lldb/test/API/tools/lldb-server/vCont-threads/main.cpp
llvm/lib/Support/PrettyStackTrace.cpp
llvm/lib/Support/Unix/Signals.inc

Removed: 




diff  --git a/lldb/test/API/tools/lldb-server/vCont-threads/main.cpp 
b/lldb/test/API/tools/lldb-server/vCont-threads/main.cpp
index 77b6aacc7d4d8..ffc1489dfeb8d 100644
--- a/lldb/test/API/tools/lldb-server/vCont-threads/main.cpp
+++ b/lldb/test/API/tools/lldb-server/vCont-threads/main.cpp
@@ -14,7 +14,7 @@
 
 pseudo_barrier_t barrier;
 std::mutex print_mutex;
-std::atomic can_work = ATOMIC_VAR_INIT(false);
+std::atomic can_work = false;
 thread_local volatile sig_atomic_t can_exit_now = false;
 
 static void sigint_handler(int signo) {}

diff  --git a/llvm/lib/Support/PrettyStackTrace.cpp 
b/llvm/lib/Support/PrettyStackTrace.cpp
index fa91405fee10a..f9f1b8a419b82 100644
--- a/llvm/lib/Support/PrettyStackTrace.cpp
+++ b/llvm/lib/Support/PrettyStackTrace.cpp
@@ -64,8 +64,7 @@ static LLVM_THREAD_LOCAL PrettyStackTraceEntry 
*PrettyStackTraceHead = nullptr;
 // the current thread". If the user happens to overflow an 'unsigned' with
 // SIGINFO requests, it's possible that some threads will stop responding to 
it,
 // but the program won't crash.
-static volatile std::atomic GlobalSigInfoGenerationCounter =
-ATOMIC_VAR_INIT(1);
+static volatile std::atomic GlobalSigInfoGenerationCounter = 1;
 static LLVM_THREAD_LOCAL unsigned ThreadLocalSigInfoGenerationCounter = 0;
 
 namespace llvm {

diff  --git a/llvm/lib/Support/Unix/Signals.inc 
b/llvm/lib/Support/Unix/Signals.inc
index 05a7335216f43..ce093d5a4dc07 100644
--- a/llvm/lib/Support/Unix/Signals.inc
+++ b/llvm/lib/Support/Unix/Signals.inc
@@ -84,13 +84,11 @@ static void InfoSignalHandler(int Sig); // defined below.
 
 using SignalHandlerFunctionType = void (*)();
 /// The function to call if ctrl-c is pressed.
-static std::atomic InterruptFunction =
-ATOMIC_VAR_INIT(nullptr);
-static std::atomic InfoSignalFunction =
-ATOMIC_VAR_INIT(nullptr);
+static std::atomic InterruptFunction = nullptr;
+static std::atomic InfoSignalFunction = nullptr;
 /// The function to call on SIGPIPE (one-time use only).
 static std::atomic OneShotPipeSignalFunction =
-ATOMIC_VAR_INIT(nullptr);
+nullptr;
 
 namespace {
 /// Signal-safe removal of files.
@@ -98,8 +96,8 @@ namespace {
 /// themselves is signal-safe. Memory is freed when the head is freed, deletion
 /// is therefore not signal-safe either.
 class FileToRemoveList {
-  std::atomic Filename = ATOMIC_VAR_INIT(nullptr);
-  std::atomic Next = ATOMIC_VAR_INIT(nullptr);
+  std::atomic Filename = nullptr;
+  std::atomic Next = nullptr;
 
   FileToRemoveList() = default;
   // Not signal-safe.
@@ -188,7 +186,7 @@ public:
 Head.exchange(OldHead);
   }
 };
-static std::atomic FilesToRemove = 
ATOMIC_VAR_INIT(nullptr);
+static std::atomic FilesToRemove = nullptr;
 
 /// Clean up the list in a signal-friendly manner.
 /// Recall that signals can fire during llvm_shutdown. If this occurs we should
@@ -248,7 +246,7 @@ static const int InfoSigs[] = {SIGUSR1
 static const size_t NumSigs = std::size(IntSigs) + std::size(KillSigs) +
   std::size(InfoSigs) + 1 /* SIGPIPE */;
 
-static std::atomic NumRegisteredSignals = ATOMIC_VAR_INIT(0);
+static std::atomic NumRegisteredSignals = 0;
 static struct {
   struct sigaction SA;
   int SigNo;



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

[Lldb-commits] [PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.



Comment at: clang/test/CXX/temp/temp.arg/temp.arg.nontype/p1.cpp:204
+#if __cplusplus == 201703L
+  // cxx17-error@-3 {{non-type template argument refers to subobject '(int 
*)1'}}
+#endif

Shouldn't this be an error b/c it is a temporary? What is `(int*)1` a subobject 
of?



Comment at: clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp:21
+using IPn = IntPtr<&n + 1>;
+using IPn = IntPtr<&n + 1>;
 

`using IPn = IntPtr<&n + 2>` should be an error since that would be out of 
bounds, while `+1` is ok b/c it is one past the end as long as we don't deref.



Comment at: clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp:21
+using IPn = IntPtr<&n + 1>;
+using IPn = IntPtr<&n + 1>;
 

shafik wrote:
> `using IPn = IntPtr<&n + 2>` should be an error since that would be out of 
> bounds, while `+1` is ok b/c it is one past the end as long as we don't deref.
gcc reject this one but I think their pointer math is wonky here: 
https://godbolt.org/z/fhMqPPefG



Comment at: clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp:26
 
-using IP3 = IntPtr<&s.n[3]>; // FIXME expected-error {{refers to subobject}}
-using IP3 = IntPtr; // FIXME expected-error {{refers to subobject}}
+using IP3 = IntPtr<&s.n[3]>;
+using IP3 = IntPtr;

We should reject `IntPtr<&s.n[5]>;` again b/c it is out of bounds. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140996

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

[Lldb-commits] [lldb] bcb8a94 - [lldb][test] Fix vCont-threads/main.cp for -std=c++11

[Fangrui Song via lldb-commits]

Author: Fangrui Song
Date: 2023-02-24T15:08:24-08:00
New Revision: bcb8a94503887250d3a818a6b631899e9233080c

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

LOG: [lldb][test] Fix vCont-threads/main.cp for -std=c++11

Added: 


Modified: 
lldb/test/API/tools/lldb-server/vCont-threads/main.cpp

Removed: 




diff  --git a/lldb/test/API/tools/lldb-server/vCont-threads/main.cpp 
b/lldb/test/API/tools/lldb-server/vCont-threads/main.cpp
index ffc1489dfeb8..28890dc96732 100644
--- a/lldb/test/API/tools/lldb-server/vCont-threads/main.cpp
+++ b/lldb/test/API/tools/lldb-server/vCont-threads/main.cpp
@@ -14,7 +14,7 @@
 
 pseudo_barrier_t barrier;
 std::mutex print_mutex;
-std::atomic can_work = false;
+std::atomic can_work;
 thread_local volatile sig_atomic_t can_exit_now = false;
 
 static void sigint_handler(int signo) {}



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

[Lldb-commits] [PATCH] D144688: [lldb] Fix {break, watch}point command function stopping behavior

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 500317.
mib retitled this revision from "[lldb] Fix watchpoint command function 
stopping behavior" to "[lldb] Fix {break,watch}point command function stopping 
behavior".
mib edited the summary of this revision.
mib added a comment.

Address @jingham & @delcypher feedbacks.


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

https://reviews.llvm.org/D144688

Files:
  lldb/source/Commands/CommandObjectWatchpointCommand.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  
lldb/test/API/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandPython.py
  lldb/test/API/commands/watchpoints/watchpoint_commands/command/main.cpp
  
lldb/test/API/commands/watchpoints/watchpoint_commands/command/watchpoint_command.py

Index: lldb/test/API/commands/watchpoints/watchpoint_commands/command/watchpoint_command.py
===
--- lldb/test/API/commands/watchpoints/watchpoint_commands/command/watchpoint_command.py
+++ lldb/test/API/commands/watchpoints/watchpoint_commands/command/watchpoint_command.py
@@ -9,7 +9,12 @@
 print ("I stopped the first time")
 frame.EvaluateExpression("cookie = 888")
 num_hits += 1
-frame.thread.process.Continue()
+return True
+if num_hits == 1:
+print ("I stopped the second time, but with no return")
+frame.EvaluateExpression("cookie = 666")
+num_hits += 1
 else:
 print ("I stopped the %d time" % (num_hits))
 frame.EvaluateExpression("cookie = 999")
+return False # This cause the process to continue.
Index: lldb/test/API/commands/watchpoints/watchpoint_commands/command/main.cpp
===
--- lldb/test/API/commands/watchpoints/watchpoint_commands/command/main.cpp
+++ lldb/test/API/commands/watchpoints/watchpoint_commands/command/main.cpp
@@ -16,5 +16,5 @@
 modify(global);
 
 printf("global=%d\n", global);
-printf("cookie=%d\n", cookie);
+printf("cookie=%d\n", cookie); // Set another breakpoint here.
 }
Index: lldb/test/API/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandPython.py
===
--- lldb/test/API/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandPython.py
+++ lldb/test/API/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandPython.py
@@ -1,4 +1,4 @@
-"""
+"""
 Test 'watchpoint command'.
 """
 
@@ -22,6 +22,8 @@
 # Find the line number to break inside main().
 self.line = line_number(
 self.source, '// Set break point at this line.')
+self.second_line = line_number(
+self.source, '// Set another breakpoint here.')
 # And the watchpoint variable declaration line number.
 self.decl = line_number(self.source,
 '// Watchpoint variable declaration.')
@@ -143,6 +145,32 @@
 self.expect("thread backtrace", STOPPED_DUE_TO_WATCHPOINT,
 substrs=['stop reason = watchpoint'])
 
+# We should have hit the watchpoint once, set cookie to 888, since the
+# user callback returned True.
+self.expect("frame variable --show-globals cookie",
+substrs=['(int32_t)', 'cookie = 888'])
+
+self.runCmd("process continue")
+
+# We should be stopped again due to the watchpoint (write type).
+# The stop reason of the thread should be watchpoint.
+self.expect("thread backtrace", STOPPED_DUE_TO_WATCHPOINT,
+substrs=['stop reason = watchpoint'])
+
+# We should have hit the watchpoint a second time, set cookie to 666,
+# even if the user callback didn't return anything and then continue.
+self.expect("frame variable --show-globals cookie",
+substrs=['(int32_t)', 'cookie = 666'])
+
+# Add a breakpoint to set a watchpoint when stopped on the breakpoint.
+lldbutil.run_break_set_by_file_and_line(
+self, None, self.second_line, num_expected_locations=1)
+
+self.runCmd("process continue")
+
+self.expect("thread backtrace", STOPPED_DUE_TO_BREAKPOINT,
+substrs=['stop reason = breakpoint'])
+
 # We should have hit the watchpoint once, set cookie to 888, then continued to the
 # second hit and set it to 999
 self.expect("frame variable --show-globals cookie",
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -1292,40 +1292,52 @@
   StringList auto_generated_function;
   auto_generated_function.Appen

[Lldb-commits] [PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.



Comment at: clang/test/CodeGenCXX/template-arguments.cpp:4
+
+template CONSTEXPR T id(T v) { return v; }
+template auto value = id(V);

I don't see any tests covering unions or enums.



Comment at: clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp:12
+using F1 = Float<1.0f>;
+using F1 = Float<2.0f / 2>;
 

I believe this is IFNDR the template-heads are functionally equivelent but not 
equivelent: https://eel.is/c++draft/temp.class#general-3



Comment at: clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp:56
+using CF = ComplexFloat<1.0f + 3.0fi>;
+using CF = ComplexFloat<3.0fi + 1.0f>;
 

Can we add an enum example e.g.:

```
enum E{ E1, E2};
template  struct SE {};
using IPE = SE;
using IPE = SE;

```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140996

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

[Lldb-commits] [PATCH] D144688: [lldb] Fix {break, watch}point command function stopping behavior

[Alex Langford via Phabricator via lldb-commits]

bulbazord requested changes to this revision.
bulbazord added inline comments.
This revision now requires changes to proceed.



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp:1314-1324
+if (line.startswith("return ")) {
+  llvm::StringRef return_val =
+  line.substr(llvm::StringRef("return ").size());
+  sstr.Printf("  __return_val = %s", return_val.data());
+} else {
+  sstr.Printf("  %s", line.data());
+}

I feel like this is subtly wrong. For example, given the user provided function:
```
def func(frame, bp_loc, internal_dict):
if frame.name == "main":
return True
return False
```

This will be transformed into:
```
def __user_callback():
nonlocal __return_val
if frame.name == "main":
__return_val = True
__return_val = False
```
Assume frame.name == "main": In the first one, we will return True. In the 
second one, `__return_val` will be False. 

Maybe we can insert the input into `__user_callback` as-is and do `__return_val 
= __user_callback()`?


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

https://reviews.llvm.org/D144688

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

[Lldb-commits] [PATCH] D144688: [lldb] Fix {break, watch}point command function stopping behavior

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp:1314-1324
+if (line.startswith("return ")) {
+  llvm::StringRef return_val =
+  line.substr(llvm::StringRef("return ").size());
+  sstr.Printf("  __return_val = %s", return_val.data());
+} else {
+  sstr.Printf("  %s", line.data());
+}

bulbazord wrote:
> I feel like this is subtly wrong. For example, given the user provided 
> function:
> ```
> def func(frame, bp_loc, internal_dict):
> if frame.name == "main":
> return True
> return False
> ```
> 
> This will be transformed into:
> ```
> def __user_callback():
> nonlocal __return_val
> if frame.name == "main":
> __return_val = True
> __return_val = False
> ```
> Assume frame.name == "main": In the first one, we will return True. In the 
> second one, `__return_val` will be False. 
> 
> Maybe we can insert the input into `__user_callback` as-is and do 
> `__return_val = __user_callback()`?
To be clear, I think this will work with `-F` because there's just one `return` 
statement inserted a few frames up.

This also may fail with `-o` if you concatenate multiple lines in one. For 
example, `-o "print('foo'); return frame.name == 'main'"`. This is one line, so 
we'll never set `__return_val` to `frame.name == 'main'`. You might need to do 
further processing on the input to break it into multiple lines.


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

https://reviews.llvm.org/D144688

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

[Lldb-commits] [PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.



Comment at: clang/include/clang/AST/TemplateBase.h:88
+/// so cannot be dependent.
+UncommonValue,
+

erichkeane wrote:
> I definitely hate the name here... Just `Value` makes a bit more sense, but 
> isn't perfectly accurate.  Perhaps `NonTypeValue`?  But that is a little 
> redundant.  `Uncommon` here is just strange and not particularly descriptive. 
This catch all `UncommonValue` feels artificial. Maybe we need a separate out 
the cases into more granular cases like `Float` etc



Comment at: clang/lib/AST/TemplateBase.cpp:204-211
+  if (Type->isIntegralOrEnumerationType() && V.isInt())
+*this = TemplateArgument(Ctx, V.getInt(), Type);
+  else if ((V.isLValue() && V.isNullPointer()) ||
+   (V.isMemberPointer() && !V.getMemberPointerDecl()))
+*this = TemplateArgument(Type, /*isNullPtr=*/true);
+  else if (const ValueDecl *VD = getAsSimpleValueDeclRef(Ctx, Type, V))
+// FIXME: The Declaration form should expose a const ValueDecl*.

erichkeane wrote:
> aaron.ballman wrote:
> > Well this is certainly a unique approach...  Personally, I think it'd be 
> > nice to not assign to `this` within a constructor by calling other 
> > constructor; that falls squarely into "wait, what, can you even DO that?" 
> > kind of questions for me.
> I agree, this function needs to be refactored to call some sort of 'init' 
> function or something, even if we have to refactor the other constructors.  
> This assignment to `*this` is just too strange to let stay.
I am going to third this sentiment, this is definitely not the right approach 
and the fact that you have this ad-hoc case below here also speaks to this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140996

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

[Lldb-commits] [PATCH] D144688: [lldb] Fix {break, watch}point command function stopping behaviour

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 500349.
mib retitled this revision from "[lldb] Fix {break,watch}point command function 
stopping behavior" to "[lldb] Fix {break,watch}point command function stopping 
behaviour".
mib edited the summary of this revision.
mib added a comment.

The third implementation is the charm 😝:

- If the user input is a oneliner, we wrap it into a nested function.
- Then we call the function (either the nested function or the user provided 
python function) and capture the return value in a variable.
- Finally, after resetting the global dictionary to its previous state, we 
return the variable that contains the user's return value.


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

https://reviews.llvm.org/D144688

Files:
  lldb/include/lldb/Interpreter/ScriptInterpreter.h
  lldb/source/Commands/CommandObjectWatchpointCommand.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
  
lldb/test/API/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandPython.py
  lldb/test/API/commands/watchpoints/watchpoint_commands/command/main.cpp
  
lldb/test/API/commands/watchpoints/watchpoint_commands/command/watchpoint_command.py

Index: lldb/test/API/commands/watchpoints/watchpoint_commands/command/watchpoint_command.py
===
--- lldb/test/API/commands/watchpoints/watchpoint_commands/command/watchpoint_command.py
+++ lldb/test/API/commands/watchpoints/watchpoint_commands/command/watchpoint_command.py
@@ -9,7 +9,12 @@
 print ("I stopped the first time")
 frame.EvaluateExpression("cookie = 888")
 num_hits += 1
-frame.thread.process.Continue()
+return True
+if num_hits == 1:
+print ("I stopped the second time, but with no return")
+frame.EvaluateExpression("cookie = 666")
+num_hits += 1
 else:
 print ("I stopped the %d time" % (num_hits))
 frame.EvaluateExpression("cookie = 999")
+return False # This cause the process to continue.
Index: lldb/test/API/commands/watchpoints/watchpoint_commands/command/main.cpp
===
--- lldb/test/API/commands/watchpoints/watchpoint_commands/command/main.cpp
+++ lldb/test/API/commands/watchpoints/watchpoint_commands/command/main.cpp
@@ -16,5 +16,5 @@
 modify(global);
 
 printf("global=%d\n", global);
-printf("cookie=%d\n", cookie);
+printf("cookie=%d\n", cookie); // Set another breakpoint here.
 }
Index: lldb/test/API/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandPython.py
===
--- lldb/test/API/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandPython.py
+++ lldb/test/API/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandPython.py
@@ -1,4 +1,4 @@
-"""
+"""
 Test 'watchpoint command'.
 """
 
@@ -22,6 +22,8 @@
 # Find the line number to break inside main().
 self.line = line_number(
 self.source, '// Set break point at this line.')
+self.second_line = line_number(
+self.source, '// Set another breakpoint here.')
 # And the watchpoint variable declaration line number.
 self.decl = line_number(self.source,
 '// Watchpoint variable declaration.')
@@ -143,6 +145,32 @@
 self.expect("thread backtrace", STOPPED_DUE_TO_WATCHPOINT,
 substrs=['stop reason = watchpoint'])
 
+# We should have hit the watchpoint once, set cookie to 888, since the
+# user callback returned True.
+self.expect("frame variable --show-globals cookie",
+substrs=['(int32_t)', 'cookie = 888'])
+
+self.runCmd("process continue")
+
+# We should be stopped again due to the watchpoint (write type).
+# The stop reason of the thread should be watchpoint.
+self.expect("thread backtrace", STOPPED_DUE_TO_WATCHPOINT,
+substrs=['stop reason = watchpoint'])
+
+# We should have hit the watchpoint a second time, set cookie to 666,
+# even if the user callback didn't return anything and then continue.
+self.expect("frame variable --show-globals cookie",
+substrs=['(int32_t)', 'cookie = 666'])
+
+# Add a breakpoint to set a watchpoint when stopped on the breakpoint.
+lldbutil.run_break_set_by_file_and_line(
+self, None, self.second_line, num_expected_locations=1)
+
+self.runCmd("process continue")
+
+self.expect("thread backtrace", STOPPED_DUE_TO_BREAKPOINT,
+substrs=['stop reason = breakpoint'])
+
 # We should have hit the watchpoint once, set cookie to 888, then continued to the
 # seco