[Lldb-commits] [PATCH] D131858: [clang] Track the templated entity in type substitution.
davrec added a comment.
I like the late changes, just need to add comments to the public methods, and
maybe move `getReplacedTemplateParameterList` over to `Decl`.
Comment at: clang/include/clang/AST/DeclTemplate.h:3427
+TemplateParameterList *getReplacedTemplateParameterList(Decl *D);
+
I don't object with making this public, and I can see the argument for making
this its own function rather than a Decl method all things being equal, but
given that we already have `Decl::getDescribedTemplateParams`, I think that
# this should probably also go in Decl,
# a good comment is needed explaining the difference between the two (e.g. that
the `D` is the template/template-like decl itself, not a templated decl as
required by `getDescribedTemplateParams`, or whatever), and what happens when
it is called on a non-template decl, and
# it probably should be named just `getTemplateParams` or something that
suggests its difference with `getDescribedTemplateParams`.
Comment at: clang/include/clang/AST/ExprCXX.h:4307
- NonTypeTemplateParmDecl *getParameter() const {
-return ParamAndRef.getPointer();
+ Decl *getAssociatedDecl() const { return AssociatedDeclAndRef.getPointer(); }
+
Add comment
Comment at: clang/include/clang/AST/ExprCXX.h:4321
+
+ bool isReferenceParameter() const { return AssociatedDeclAndRef.getInt(); }
(Not your responsibility but while you're adding comments maybe you could add
one for this too.)
Comment at: clang/include/clang/AST/ExprCXX.h:4378-4380
+ Decl *getAssociatedDecl() const { return AssociatedDecl; }
+
+ unsigned getIndex() const { return Index; }
Add comments
Comment at: clang/include/clang/AST/TemplateName.h:153-154
+
+ Decl *getAssociatedDecl() const { return AssociatedDecl; }
+ unsigned getIndex() const { return Bits.Index; }
Add comments
Comment at: clang/include/clang/AST/TemplateName.h:386-387
public:
- TemplateTemplateParmDecl *getParameter() const { return Parameter; }
+ Decl *getAssociatedDecl() const { return AssociatedDecl; }
+ unsigned getIndex() const { return Bits.Index; }
+
Add comments
Comment at: clang/lib/AST/DeclTemplate.cpp:1610
+ default:
+llvm_unreachable("Unhandled templated declaration kind");
+ }
Now that this is public, probably should return nullptr - otherwise users need
the same huge switch just to decide whether they can call this function.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D131858/new/
https://reviews.llvm.org/D131858
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D131858: [clang] Track the templated entity in type substitution.
davrec added inline comments. Comment at: clang/include/clang/AST/DeclTemplate.h:3427 +TemplateParameterList *getReplacedTemplateParameterList(Decl *D); + davrec wrote: > I don't object with making this public, and I can see the argument for making > this its own function rather than a Decl method all things being equal, but > given that we already have `Decl::getDescribedTemplateParams`, I think that > # this should probably also go in Decl, > # a good comment is needed explaining the difference between the two (e.g. > that the `D` is the template/template-like decl itself, not a templated decl > as required by `getDescribedTemplateParams`, or whatever), and what happens > when it is called on a non-template decl, and > # it probably should be named just `getTemplateParams` or something that > suggests its difference with `getDescribedTemplateParams`. > On second thought this should definitely be a Decl method called `getTemplateParameters()`, since all it does is dispatches to the derived class's implementation of that. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131858/new/ https://reviews.llvm.org/D131858 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D131858: [clang] Track the templated entity in type substitution.
mizvekov added inline comments. Comment at: clang/include/clang/AST/DeclTemplate.h:3427 +TemplateParameterList *getReplacedTemplateParameterList(Decl *D); + davrec wrote: > davrec wrote: > > I don't object with making this public, and I can see the argument for > > making this its own function rather than a Decl method all things being > > equal, but given that we already have `Decl::getDescribedTemplateParams`, I > > think that > > # this should probably also go in Decl, > > # a good comment is needed explaining the difference between the two (e.g. > > that the `D` is the template/template-like decl itself, not a templated > > decl as required by `getDescribedTemplateParams`, or whatever), and what > > happens when it is called on a non-template decl, and > > # it probably should be named just `getTemplateParams` or something that > > suggests its difference with `getDescribedTemplateParams`. > > > On second thought this should definitely be a Decl method called > `getTemplateParameters()`, since all it does is dispatches to the derived > class's implementation of that. I think this function shouldn't be public in that sense, it should only be used for implementation of retrieving parameter for Subst* nodes. I don't think this should try to handle any other kinds of decls which can't appear as the AssociatedDecl in a Subst node. There will be Subst specific changes here in the future, but which depend on some other enablers: * We need to make Subst nodes for variable templates store the SpecializationDecl instead of the TemplateDecl, but this requires changing the order between creating the specialization and performing the substitution. I will do that in a separate patch. * We will stop creating Subst* nodes for things we already performed substitution with sugar. And so, we won't need to handle alias templates, conceptdecls, etc. Subst nodes will only be used for things which we track specializations for, and that need resugaring. It's only made 'public' because now we are using it across far away places like `Type.cpp` and `ExprCXX.cpp` and `TemplateName.cpp`. I didn't think this needs to go as a Decl member, because of limited scope and because it only ever needs to access public members. I don't think this justifies either a separate header to be included only where it's needed. One option is to further make the name more specific, by adding `Internal` to the name for example. Any other ideas? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131858/new/ https://reviews.llvm.org/D131858 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D131858: [clang] Track the templated entity in type substitution.
mizvekov added inline comments. Comment at: clang/include/clang/AST/DeclTemplate.h:3427 +TemplateParameterList *getReplacedTemplateParameterList(Decl *D); + mizvekov wrote: > davrec wrote: > > davrec wrote: > > > I don't object with making this public, and I can see the argument for > > > making this its own function rather than a Decl method all things being > > > equal, but given that we already have `Decl::getDescribedTemplateParams`, > > > I think that > > > # this should probably also go in Decl, > > > # a good comment is needed explaining the difference between the two > > > (e.g. that the `D` is the template/template-like decl itself, not a > > > templated decl as required by `getDescribedTemplateParams`, or whatever), > > > and what happens when it is called on a non-template decl, and > > > # it probably should be named just `getTemplateParams` or something that > > > suggests its difference with `getDescribedTemplateParams`. > > > > > On second thought this should definitely be a Decl method called > > `getTemplateParameters()`, since all it does is dispatches to the derived > > class's implementation of that. > I think this function shouldn't be public in that sense, it should only be > used for implementation of retrieving parameter for Subst* nodes. > > I don't think this should try to handle any other kinds of decls which can't > appear as the AssociatedDecl in a Subst node. > > There will be Subst specific changes here in the future, but which depend on > some other enablers: > * We need to make Subst nodes for variable templates store the > SpecializationDecl instead of the TemplateDecl, but this requires changing > the order between creating the specialization and performing the > substitution. I will do that in a separate patch. > * We will stop creating Subst* nodes for things we already performed > substitution with sugar. And so, we won't need to handle alias templates, > conceptdecls, etc. Subst nodes will only be used for things which we track > specializations for, and that need resugaring. > > It's only made 'public' because now we are using it across far away places > like `Type.cpp` and `ExprCXX.cpp` and `TemplateName.cpp`. > > I didn't think this needs to go as a Decl member, because of limited scope > and because it only ever needs to access public members. > I don't think this justifies either a separate header to be included only > where it's needed. > > One option is to further make the name more specific, by adding `Internal` to > the name for example. > Any other ideas? I ended up simply documenting that this is an internal function, for now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131858/new/ https://reviews.llvm.org/D131858 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D131858: [clang] Track the templated entity in type substitution.
davrec added inline comments. Comment at: clang/include/clang/AST/DeclTemplate.h:3427 +TemplateParameterList *getReplacedTemplateParameterList(Decl *D); + mizvekov wrote: > mizvekov wrote: > > davrec wrote: > > > davrec wrote: > > > > I don't object with making this public, and I can see the argument for > > > > making this its own function rather than a Decl method all things being > > > > equal, but given that we already have > > > > `Decl::getDescribedTemplateParams`, I think that > > > > # this should probably also go in Decl, > > > > # a good comment is needed explaining the difference between the two > > > > (e.g. that the `D` is the template/template-like decl itself, not a > > > > templated decl as required by `getDescribedTemplateParams`, or > > > > whatever), and what happens when it is called on a non-template decl, > > > > and > > > > # it probably should be named just `getTemplateParams` or something > > > > that suggests its difference with `getDescribedTemplateParams`. > > > > > > > On second thought this should definitely be a Decl method called > > > `getTemplateParameters()`, since all it does is dispatches to the derived > > > class's implementation of that. > > I think this function shouldn't be public in that sense, it should only be > > used for implementation of retrieving parameter for Subst* nodes. > > > > I don't think this should try to handle any other kinds of decls which > > can't appear as the AssociatedDecl in a Subst node. > > > > There will be Subst specific changes here in the future, but which depend > > on some other enablers: > > * We need to make Subst nodes for variable templates store the > > SpecializationDecl instead of the TemplateDecl, but this requires changing > > the order between creating the specialization and performing the > > substitution. I will do that in a separate patch. > > * We will stop creating Subst* nodes for things we already performed > > substitution with sugar. And so, we won't need to handle alias templates, > > conceptdecls, etc. Subst nodes will only be used for things which we track > > specializations for, and that need resugaring. > > > > It's only made 'public' because now we are using it across far away places > > like `Type.cpp` and `ExprCXX.cpp` and `TemplateName.cpp`. > > > > I didn't think this needs to go as a Decl member, because of limited scope > > and because it only ever needs to access public members. > > I don't think this justifies either a separate header to be included only > > where it's needed. > > > > One option is to further make the name more specific, by adding `Internal` > > to the name for example. > > Any other ideas? > I ended up simply documenting that this is an internal function, for now. That works for me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131858/new/ https://reviews.llvm.org/D131858 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] 921a4d5 - [lldb] Use std::underlying_type_t (NFC)
Author: Kazu Hirata
Date: 2022-10-15T12:11:56-07:00
New Revision: 921a4d5be4bbe3c337419dfcc313b3eb892c2870
URL:
https://github.com/llvm/llvm-project/commit/921a4d5be4bbe3c337419dfcc313b3eb892c2870
DIFF:
https://github.com/llvm/llvm-project/commit/921a4d5be4bbe3c337419dfcc313b3eb892c2870.diff
LOG: [lldb] Use std::underlying_type_t (NFC)
Added:
Modified:
lldb/include/lldb/lldb-enumerations.h
lldb/source/Breakpoint/BreakpointResolverName.cpp
Removed:
diff --git a/lldb/include/lldb/lldb-enumerations.h
b/lldb/include/lldb/lldb-enumerations.h
index 2ac1a74214b4..9f1183a06b53 100644
--- a/lldb/include/lldb/lldb-enumerations.h
+++ b/lldb/include/lldb/lldb-enumerations.h
@@ -20,18 +20,15 @@
// this entire block, as it is not necessary for swig processing.
#define LLDB_MARK_AS_BITMASK_ENUM(Enum)
\
constexpr Enum operator|(Enum a, Enum b) {
\
-return static_cast(
\
-static_cast::type>(a) |
\
-static_cast::type>(b));
\
+return static_cast(static_cast>(a) |
\
+ static_cast>(b));
\
}
\
constexpr Enum operator&(Enum a, Enum b) {
\
-return static_cast(
\
-static_cast::type>(a) &
\
-static_cast::type>(b));
\
+return static_cast(static_cast>(a) &
\
+ static_cast>(b));
\
}
\
constexpr Enum operator~(Enum a) {
\
-return static_cast(
\
-~static_cast::type>(a));
\
+return static_cast(~static_cast>(a));
\
}
\
inline Enum &operator|=(Enum &a, Enum b) {
\
a = a | b;
\
diff --git a/lldb/source/Breakpoint/BreakpointResolverName.cpp
b/lldb/source/Breakpoint/BreakpointResolverName.cpp
index dbaeec9c9afb..5f42a96cd8c2 100644
--- a/lldb/source/Breakpoint/BreakpointResolverName.cpp
+++ b/lldb/source/Breakpoint/BreakpointResolverName.cpp
@@ -164,7 +164,7 @@ BreakpointResolver
*BreakpointResolverName::CreateFromStructuredData(
error.SetErrorString("BRN::CFSD: name entry is not a string.");
return nullptr;
}
- std::underlying_type::type fnt;
+ std::underlying_type_t fnt;
success = names_mask_array->GetItemAtIndexAsInteger(i, fnt);
if (!success) {
error.SetErrorString("BRN::CFSD: name mask entry is not an integer.");
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] 39f0124 - [lldb] Fix a warning
Author: Kazu Hirata
Date: 2022-10-15T12:32:20-07:00
New Revision: 39f01240e76678fa6385d36e7a96670677a26d65
URL:
https://github.com/llvm/llvm-project/commit/39f01240e76678fa6385d36e7a96670677a26d65
DIFF:
https://github.com/llvm/llvm-project/commit/39f01240e76678fa6385d36e7a96670677a26d65.diff
LOG: [lldb] Fix a warning
This patch fixes:
lldb/source/Commands/CommandObjectThread.cpp:66:61: warning:
comparison of unsigned expression in ‘< 0’ is always false
[-Wtype-limits]
Added:
Modified:
lldb/source/Commands/CommandObjectThread.cpp
Removed:
diff --git a/lldb/source/Commands/CommandObjectThread.cpp
b/lldb/source/Commands/CommandObjectThread.cpp
index 7b739fef0a7a4..2a22900793ce5 100644
--- a/lldb/source/Commands/CommandObjectThread.cpp
+++ b/lldb/source/Commands/CommandObjectThread.cpp
@@ -63,7 +63,7 @@ class CommandObjectThreadBacktrace : public
CommandObjectIterateOverThreads {
switch (short_option) {
case 'c':
-if (option_arg.getAsInteger(0, m_count) || (m_count < 0)) {
+if (option_arg.getAsInteger(0, m_count)) {
m_count = UINT32_MAX;
error.SetErrorStringWithFormat(
"invalid integer value for option '%c'", short_option);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D136011: [lldb] Don't check environment default char signedness when creating clang type for "char"
dblaikie added a comment. I think the place where this will go wrong is in terms of how lldb renders `char` values on non-default-char-signedness programs (it'll render them as the default-char-signedness, which might be confusing to a user - since they'll be looking at literals, etc, that are the other signedness) and how lldb will interpret char literals (though that'll already be wrong - since the literals are already being parsed with the default-char-signedness, I think). I'm curious why there were all those expected failures re: PR23069. Were they not using the default char signedness? Or is the test using explicit signedness, and so whatever platforms happen not to have the explicit value sa their default are/were failing? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136011/new/ https://reviews.llvm.org/D136011 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D133036: [InstCombine] Treat passing undef to noundef params as UB
nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.
The default switch has happened, so unblocking this.
Comment at: clang/test/CodeGenOpenCL/overload.cl:23
generic int *local *genloc;
generic int *global *genglob;
+ // CHECK-DAG: call spir_func void @_Z3fooPU3AS1iS0_(i32 addrspace(1)*
noundef {{.*}}, i32 addrspace(1)* noundef {{.*}})
Maybe initialize the relevant variables instead? I'd assume just NULL would
work here.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D133036/new/
https://reviews.llvm.org/D133036
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D136034: [lldb][trace] Add a basic function call dump [3] - Add a JSON dumper
wallace created this revision.
wallace added reviewers: persona0220, jj10306.
Herald added a project: All.
wallace requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
The JSON dumper is very minimalistic. It pretty much only shows the
delimiting instruction IDs of every segment, so that further queries to
the SBCursor can be used to make sense of the data. It's main purpose is
to be serialized somewhat cheaply.
I also renamed untracedSegment to untracedPrefixSegment, in case in the
future we add an untracedSuffixSegment. In any case, this new name is
more explicit, which I like.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D136034
Files:
lldb/include/lldb/Target/TraceDumper.h
lldb/source/Commands/CommandObjectThread.cpp
lldb/source/Target/TraceDumper.cpp
lldb/test/API/commands/trace/TestTraceDumpFunctionCalls.py
Index: lldb/test/API/commands/trace/TestTraceDumpFunctionCalls.py
===
--- lldb/test/API/commands/trace/TestTraceDumpFunctionCalls.py
+++ lldb/test/API/commands/trace/TestTraceDumpFunctionCalls.py
@@ -10,8 +10,20 @@
self.expect("thread trace dump function-calls 2",
error=True, substrs=['error: no thread with index: "2"'])
- # We don't support yet JSON
- self.expect("thread trace dump function-calls 1 -j", substrs=['[]'])
+ self.expect("thread trace dump function-calls 1 -j",
+substrs=['[{"tracedSegments":[{"firstInstructionId":"1","lastInstructionId":"22"}]}]'])
+
+ self.expect("thread trace dump function-calls 1 -J",
+substrs=['''[
+ {
+"tracedSegments": [
+ {
+"firstInstructionId": "1",
+"lastInstructionId": "22"
+ }
+]
+ }
+]'''])
# We test first some code without function call
self.expect("thread trace dump function-calls 1",
@@ -34,6 +46,26 @@
[call tree #1]
[19532, 19532]'''])
+ self.expect("thread trace dump function-calls 2 -J",
+substrs=['''[
+ {
+"tracedSegments": [
+ {
+"firstInstructionId": "4",
+"lastInstructionId": "19524"
+ }
+]
+ },
+ {
+"tracedSegments": [
+ {
+"firstInstructionId": "19532",
+"lastInstructionId": "19532"
+ }
+]
+ }
+]'''])
+
self.expect("thread trace dump function-calls 3",
substrs=['''thread #3: tid = 3497497
@@ -43,6 +75,26 @@
[call tree #1]
[61834, 61834]'''])
+ self.expect("thread trace dump function-calls 3 -J",
+substrs=['''[
+ {
+"tracedSegments": [
+ {
+"firstInstructionId": "5",
+"lastInstructionId": "61831"
+ }
+]
+ },
+ {
+"tracedSegments": [
+ {
+"firstInstructionId": "61834",
+"lastInstructionId": "61834"
+ }
+]
+ }
+]'''])
+
def testInlineFunctionCalls(self):
self.expect("file " + os.path.join(self.getSourceDir(), "inline-function", "a.out"))
self.expect("b main") # we'll trace from the beginning of main
@@ -58,6 +110,42 @@
a.out`foo(int) + 49 at inline.cpp:9:15 to 12:1 [22, 26]
a.out`main + 25 at inline.cpp:16:14 to 16:14 [27, 27]'''])
+ self.expect("thread trace dump function-calls -J",
+substrs=['''[
+ {
+"tracedSegments": [
+ {
+"firstInstructionId": "1",
+"lastInstructionId": "5",
+"nestedCall": {
+ "tracedSegments": [
+{
+ "firstInstructionId": "6",
+ "lastInstructionId": "13",
+ "nestedCall": {
+"tracedSegments": [
+ {
+"firstInstructionId": "14",
+"lastInstructionId": "21"
+ }
+]
+ }
+},
+{
+ "firstInstructionId": "22",
+ "lastInstructionId": "26"
+}
+ ]
+}
+ },
+ {
+"firstInstructionId": "27",
+"lastInstructionId": "27"
+ }
+]
+ }
+]'''])
+
def testIncompleteInlineFunctionCalls(self):
self.expect("file " + os.path.join(self.getSourceDir(), "inline-function", "a.out"))
self.expect("b 4") # we'll trace from the middle of the inline method
@@ -73,6 +161,38 @@
a.out`foo(int) + 49 at inline.cpp:9:15 to 12:1 [6, 10]
a.out`main + 25 at inline.cpp:16:14 to 16:14 [11, 11]'''])
+ self.expect("thread trace dump function-calls -J",
+substrs=['''[
+ {
+"untracedPrefixSegment": {
+ "nestedCall": {
+"untracedPrefixSegment": {
+ "nestedCall": {
+"tracedSegments": [
+ {
+"firstInstructionId": "1",
+"lastInstructionId": "5"
+ }
+]
+ }
+},
+"tracedSegments": [
+ {
+"firstInstructionId": "6",
+"lastInstructionId": "10"
+ }
+]
+ }
