[Lldb-commits] [PATCH] D46005: [test] Add a utility for dumping all tests in the dotest suite

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D46005#1077016, @zturner wrote:

> Note that there's currently no precedent (that i'm aware of anwyay) in LLVM 
> or any of its subprojects for splitting the running of a single file into 
> multiple parallel jobs.  All of LLVM's other projects parallelize at file 
> granularity, and so it's up to to the person writing tests to ensure that the 
> file granularity matches that with which they want tests to be parallelized 
> at.


There's always gtest. Regardless of whether you consider a .cpp file or the 
built exectable to be a "test file", it will generally contain a number of 
tests. And arguably, our test suite is more similar to the gtest suite than the 
traditional lit (ShTest) suites (for one, it hijacks a third party unit testing 
library, and then tries to make it run under lit). And we run all gtest tests 
individually (I've often wondered what kind of performance impact that has, but 
they seem to be running fast enough anyway...).

For what it's worth, paralelization is not my motivation here. There some tests 
which run disproportionately long, and this will speed them up, but that may be 
offset by the fact we need to start more work this way (if anyone is interested 
I can try to do some measurements). My main reason for doing this is so that we 
can have better mapping of test result states. As it stands now, we only have 
two possible results for a test: passed or failed. Lit has more results than 
that, and they roughly match the existing dotest states (the different 
treatment of XFAILs is the biggest difference), so it should be possible to 
represent them with more fidelity. However, right now it's not possible to 
translate them reasonably, as a "single test" can result in any number of 
fails/xfails/skips/... (or no results at all).

> That doesn't mean it's not possible (as you've shown here), but it adds some 
> additional complexity and I'm not sure it's worth it.

It adds complexity, but not much imho. I was actually pleasantly surprised at 
how easy it was to pull this off. The entire implementation is 78 LOC right 
now. The changes to the test format will probably push it over a 100, but not 
by much.

That said, I am not interested in forcing this onto everyone. I did it because 
it seemed like a nice thing to have and I was curious if it is doable (easily). 
However, if there's no interest for it, then I can just abandon it..


https://reviews.llvm.org/D46005



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

[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

This code itself looks fine, I have just two minor comments.

However, I do have a question about performance. I remember us being very 
worried about performance in the past, so we ended up putting in this like 
r298876. This removes the normalization step during FileSpec comparison, but it 
introduces mandatory normalization upon every FileSpec creation, so it's not 
obvious to me what will this do to performance. Should we try to benchmark this 
somehow?

Jim, you seem to have encountered a case when the normalization was a 
bottleneck (r298876). Do you remember what situation was that in?




Comment at: source/Utility/FileSpec.cpp:245
 
+  llvm::StringRef resolve_path_ref2(resolved.c_str());
+

It looks like this is unused.



Comment at: source/Utility/FileSpec.cpp:269-270
+  else {
+// Make sure we don't end up with "." in both the directory and filename.
+// If we do, clear the directory. 
+m_filename.SetString(".");

`remove_dots` should never produce a path like this, so we should be able to 
revert this now.


https://reviews.llvm.org/D45977



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

[Lldb-commits] [lldb] r330823 - Fix -Wswitch warning after r330790.

[Benjamin Kramer via lldb-commits]

Author: d0k
Date: Wed Apr 25 06:22:47 2018
New Revision: 330823

URL: http://llvm.org/viewvc/llvm-project?rev=330823&view=rev
Log:
Fix -Wswitch warning after r330790.

source/Symbol/ClangASTContext.cpp:391:13: error: enumeration value 'HIP' not 
handled in switch [-Werror,-Wswitch]
switch (IK.getLanguage()) {

Modified:
lldb/trunk/source/Symbol/ClangASTContext.cpp

Modified: lldb/trunk/source/Symbol/ClangASTContext.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Symbol/ClangASTContext.cpp?rev=330823&r1=330822&r2=330823&view=diff
==
--- lldb/trunk/source/Symbol/ClangASTContext.cpp (original)
+++ lldb/trunk/source/Symbol/ClangASTContext.cpp Wed Apr 25 06:22:47 2018
@@ -408,6 +408,9 @@ static void ParseLangArgs(LangOptions &O
 case InputKind::ObjCXX:
   LangStd = LangStandard::lang_gnucxx98;
   break;
+case InputKind::HIP:
+  LangStd = LangStandard::lang_hip;
+  break;
 }
   }
 


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

Re: [Lldb-commits] [PATCH] D46005: [test] Add a utility for dumping all tests in the dotest suite

[Zachary Turner via lldb-commits]

Well let’s see what Davide and Adrian think. I’m more of an outsider these
days so consider my perspective an llvm-centric one, which would sometimes
(but not always) be the best for lldb
On Wed, Apr 25, 2018 at 12:31 AM Pavel Labath via Phabricator <
revi...@reviews.llvm.org> wrote:

> labath added a comment.
>
> In https://reviews.llvm.org/D46005#1077016, @zturner wrote:
>
> > Note that there's currently no precedent (that i'm aware of anwyay) in
> LLVM or any of its subprojects for splitting the running of a single file
> into multiple parallel jobs.  All of LLVM's other projects parallelize at
> file granularity, and so it's up to to the person writing tests to ensure
> that the file granularity matches that with which they want tests to be
> parallelized at.
>
>
> There's always gtest. Regardless of whether you consider a .cpp file or
> the built exectable to be a "test file", it will generally contain a number
> of tests. And arguably, our test suite is more similar to the gtest suite
> than the traditional lit (ShTest) suites (for one, it hijacks a third party
> unit testing library, and then tries to make it run under lit). And we run
> all gtest tests individually (I've often wondered what kind of performance
> impact that has, but they seem to be running fast enough anyway...).
>
> For what it's worth, paralelization is not my motivation here. There some
> tests which run disproportionately long, and this will speed them up, but
> that may be offset by the fact we need to start more work this way (if
> anyone is interested I can try to do some measurements). My main reason for
> doing this is so that we can have better mapping of test result states. As
> it stands now, we only have two possible results for a test: passed or
> failed. Lit has more results than that, and they roughly match the existing
> dotest states (the different treatment of XFAILs is the biggest
> difference), so it should be possible to represent them with more fidelity.
> However, right now it's not possible to translate them reasonably, as a
> "single test" can result in any number of fails/xfails/skips/... (or no
> results at all).
>
> > That doesn't mean it's not possible (as you've shown here), but it adds
> some additional complexity and I'm not sure it's worth it.
>
> It adds complexity, but not much imho. I was actually pleasantly surprised
> at how easy it was to pull this off. The entire implementation is 78 LOC
> right now. The changes to the test format will probably push it over a 100,
> but not by much.
>
> That said, I am not interested in forcing this onto everyone. I did it
> because it seemed like a nice thing to have and I was curious if it is
> doable (easily). However, if there's no interest for it, then I can just
> abandon it..
>
>
> https://reviews.llvm.org/D46005
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D46005: [test] Add a utility for dumping all tests in the dotest suite

[Zachary Turner via Phabricator via lldb-commits]

zturner added a subscriber: labath.
zturner added a comment.

Well let’s see what Davide and Adrian think. I’m more of an outsider these
days so consider my perspective an llvm-centric one, which would sometimes
(but not always) be the best for lldb


https://reviews.llvm.org/D46005



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

Re: [Lldb-commits] [PATCH] D46005: [test] Add a utility for dumping all tests in the dotest suite

[Adrian Prantl via lldb-commits]

Disregarding the added complexity I believe that being able to communicate the 
number of tests filed/passed inside a single file would be very valuable. For 
example when one test gets broken by upstream clang changes and it takes a few 
days to fix it properly, we don't want to loose signal on the other tests in 
the same file during that period. I think it's worth it.

-- adrian

> On Apr 25, 2018, at 8:13 AM, Zachary Turner  wrote:
> 
> Well let’s see what Davide and Adrian think. I’m more of an outsider these 
> days so consider my perspective an llvm-centric one, which would sometimes 
> (but not always) be the best for lldb
> On Wed, Apr 25, 2018 at 12:31 AM Pavel Labath via Phabricator 
> mailto:revi...@reviews.llvm.org>> wrote:
> labath added a comment.
> 
> In https://reviews.llvm.org/D46005#1077016 
> , @zturner wrote:
> 
> > Note that there's currently no precedent (that i'm aware of anwyay) in LLVM 
> > or any of its subprojects for splitting the running of a single file into 
> > multiple parallel jobs.  All of LLVM's other projects parallelize at file 
> > granularity, and so it's up to to the person writing tests to ensure that 
> > the file granularity matches that with which they want tests to be 
> > parallelized at.
> 
> 
> There's always gtest. Regardless of whether you consider a .cpp file or the 
> built exectable to be a "test file", it will generally contain a number of 
> tests. And arguably, our test suite is more similar to the gtest suite than 
> the traditional lit (ShTest) suites (for one, it hijacks a third party unit 
> testing library, and then tries to make it run under lit). And we run all 
> gtest tests individually (I've often wondered what kind of performance impact 
> that has, but they seem to be running fast enough anyway...).
> 
> For what it's worth, paralelization is not my motivation here. There some 
> tests which run disproportionately long, and this will speed them up, but 
> that may be offset by the fact we need to start more work this way (if anyone 
> is interested I can try to do some measurements). My main reason for doing 
> this is so that we can have better mapping of test result states. As it 
> stands now, we only have two possible results for a test: passed or failed. 
> Lit has more results than that, and they roughly match the existing dotest 
> states (the different treatment of XFAILs is the biggest difference), so it 
> should be possible to represent them with more fidelity. However, right now 
> it's not possible to translate them reasonably, as a "single test" can result 
> in any number of fails/xfails/skips/... (or no results at all).
> 
> > That doesn't mean it's not possible (as you've shown here), but it adds 
> > some additional complexity and I'm not sure it's worth it.
> 
> It adds complexity, but not much imho. I was actually pleasantly surprised at 
> how easy it was to pull this off. The entire implementation is 78 LOC right 
> now. The changes to the test format will probably push it over a 100, but not 
> by much.
> 
> That said, I am not interested in forcing this onto everyone. I did it 
> because it seemed like a nice thing to have and I was curious if it is doable 
> (easily). However, if there's no interest for it, then I can just abandon it..
> 
> 
> https://reviews.llvm.org/D46005 
> 
> 
> 

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

Re: [Lldb-commits] [PATCH] D46005: [test] Add a utility for dumping all tests in the dotest suite

[Zachary Turner via lldb-commits]

But is there a reason why that is more valuable with LLDB than it is with
LLVM?  In LLVM when a test fails it stops and doesn't run subsequent run
lines.  So you have the same issue there.  The way this is handled in LLVM
is that if you think tests are sufficiently different that they could be
broken by different types of changes, you split them up into different
files.

Do you feel LLDB is fundamentally different in this regard?

On Wed, Apr 25, 2018 at 8:27 AM Adrian Prantl  wrote:

> Disregarding the added complexity I believe that being able to communicate
> the number of tests filed/passed inside a single file would be very
> valuable. For example when one test gets broken by upstream clang changes
> and it takes a few days to fix it properly, we don't want to loose signal
> on the other tests in the same file during that period. I think it's worth
> it.
>
> -- adrian
>
>
> On Apr 25, 2018, at 8:13 AM, Zachary Turner  wrote:
>
> Well let’s see what Davide and Adrian think. I’m more of an outsider these
> days so consider my perspective an llvm-centric one, which would sometimes
> (but not always) be the best for lldb
> On Wed, Apr 25, 2018 at 12:31 AM Pavel Labath via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> labath added a comment.
>>
>> In https://reviews.llvm.org/D46005#1077016, @zturner wrote:
>>
>> > Note that there's currently no precedent (that i'm aware of anwyay) in
>> LLVM or any of its subprojects for splitting the running of a single file
>> into multiple parallel jobs.  All of LLVM's other projects parallelize at
>> file granularity, and so it's up to to the person writing tests to ensure
>> that the file granularity matches that with which they want tests to be
>> parallelized at.
>>
>>
>> There's always gtest. Regardless of whether you consider a .cpp file or
>> the built exectable to be a "test file", it will generally contain a number
>> of tests. And arguably, our test suite is more similar to the gtest suite
>> than the traditional lit (ShTest) suites (for one, it hijacks a third party
>> unit testing library, and then tries to make it run under lit). And we run
>> all gtest tests individually (I've often wondered what kind of performance
>> impact that has, but they seem to be running fast enough anyway...).
>>
>> For what it's worth, paralelization is not my motivation here. There some
>> tests which run disproportionately long, and this will speed them up, but
>> that may be offset by the fact we need to start more work this way (if
>> anyone is interested I can try to do some measurements). My main reason for
>> doing this is so that we can have better mapping of test result states. As
>> it stands now, we only have two possible results for a test: passed or
>> failed. Lit has more results than that, and they roughly match the existing
>> dotest states (the different treatment of XFAILs is the biggest
>> difference), so it should be possible to represent them with more fidelity.
>> However, right now it's not possible to translate them reasonably, as a
>> "single test" can result in any number of fails/xfails/skips/... (or no
>> results at all).
>>
>> > That doesn't mean it's not possible (as you've shown here), but it adds
>> some additional complexity and I'm not sure it's worth it.
>>
>> It adds complexity, but not much imho. I was actually pleasantly
>> surprised at how easy it was to pull this off. The entire implementation is
>> 78 LOC right now. The changes to the test format will probably push it over
>> a 100, but not by much.
>>
>> That said, I am not interested in forcing this onto everyone. I did it
>> because it seemed like a nice thing to have and I was curious if it is
>> doable (easily). However, if there's no interest for it, then I can just
>> abandon it..
>>
>>
>> https://reviews.llvm.org/D46005
>>
>>
>>
>>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D46005: [test] Add a utility for dumping all tests in the dotest suite

[Adrian Prantl via lldb-commits]

> On Apr 25, 2018, at 9:08 AM, Zachary Turner  wrote:
> 
> But is there a reason why that is more valuable with LLDB than it is with 
> LLVM?  In LLVM when a test fails it stops and doesn't run subsequent run 
> lines.  So you have the same issue there. 

That's not a good analogy. Multiple RUN lines don't necessarily mean separate 
tests, they mean separate commands being executed that may depend on each 
other. As Pavel pointed out before, this would be closer to how gtests behave.

-- adrian

> The way this is handled in LLVM is that if you think tests are sufficiently 
> different that they could be broken by different types of changes, you split 
> them up into different files.
> 
> Do you feel LLDB is fundamentally different in this regard?
> 
> On Wed, Apr 25, 2018 at 8:27 AM Adrian Prantl  > wrote:
> Disregarding the added complexity I believe that being able to communicate 
> the number of tests filed/passed inside a single file would be very valuable. 
> For example when one test gets broken by upstream clang changes and it takes 
> a few days to fix it properly, we don't want to loose signal on the other 
> tests in the same file during that period. I think it's worth it.
> 
> -- adrian
> 
> 
>> On Apr 25, 2018, at 8:13 AM, Zachary Turner > > wrote:
>> 
>> Well let’s see what Davide and Adrian think. I’m more of an outsider these 
>> days so consider my perspective an llvm-centric one, which would sometimes 
>> (but not always) be the best for lldb
>> On Wed, Apr 25, 2018 at 12:31 AM Pavel Labath via Phabricator 
>> mailto:revi...@reviews.llvm.org>> wrote:
>> labath added a comment.
>> 
>> In https://reviews.llvm.org/D46005#1077016 
>> , @zturner wrote:
>> 
>> > Note that there's currently no precedent (that i'm aware of anwyay) in 
>> > LLVM or any of its subprojects for splitting the running of a single file 
>> > into multiple parallel jobs.  All of LLVM's other projects parallelize at 
>> > file granularity, and so it's up to to the person writing tests to ensure 
>> > that the file granularity matches that with which they want tests to be 
>> > parallelized at.
>> 
>> 
>> There's always gtest. Regardless of whether you consider a .cpp file or the 
>> built exectable to be a "test file", it will generally contain a number of 
>> tests. And arguably, our test suite is more similar to the gtest suite than 
>> the traditional lit (ShTest) suites (for one, it hijacks a third party unit 
>> testing library, and then tries to make it run under lit). And we run all 
>> gtest tests individually (I've often wondered what kind of performance 
>> impact that has, but they seem to be running fast enough anyway...).
>> 
>> For what it's worth, paralelization is not my motivation here. There some 
>> tests which run disproportionately long, and this will speed them up, but 
>> that may be offset by the fact we need to start more work this way (if 
>> anyone is interested I can try to do some measurements). My main reason for 
>> doing this is so that we can have better mapping of test result states. As 
>> it stands now, we only have two possible results for a test: passed or 
>> failed. Lit has more results than that, and they roughly match the existing 
>> dotest states (the different treatment of XFAILs is the biggest difference), 
>> so it should be possible to represent them with more fidelity. However, 
>> right now it's not possible to translate them reasonably, as a "single test" 
>> can result in any number of fails/xfails/skips/... (or no results at all).
>> 
>> > That doesn't mean it's not possible (as you've shown here), but it adds 
>> > some additional complexity and I'm not sure it's worth it.
>> 
>> It adds complexity, but not much imho. I was actually pleasantly surprised 
>> at how easy it was to pull this off. The entire implementation is 78 LOC 
>> right now. The changes to the test format will probably push it over a 100, 
>> but not by much.
>> 
>> That said, I am not interested in forcing this onto everyone. I did it 
>> because it seemed like a nice thing to have and I was curious if it is 
>> doable (easily). However, if there's no interest for it, then I can just 
>> abandon it..
>> 
>> 
>> https://reviews.llvm.org/D46005 
>> 
>> 
>> 
> 

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

Re: [Lldb-commits] [PATCH] D46005: [test] Add a utility for dumping all tests in the dotest suite

[Zachary Turner via lldb-commits]

Sure multiple run lines can be commands being executed that depend on each
other, but it's very common for a test to have multiple check labels as well

On Wed, Apr 25, 2018 at 9:19 AM Adrian Prantl  wrote:

>
> On Apr 25, 2018, at 9:08 AM, Zachary Turner  wrote:
>
> But is there a reason why that is more valuable with LLDB than it is with
> LLVM?  In LLVM when a test fails it stops and doesn't run subsequent run
> lines.  So you have the same issue there.
>
>
> That's not a good analogy. Multiple RUN lines don't necessarily mean
> separate tests, they mean separate commands being executed that may depend
> on each other. As Pavel pointed out before, this would be closer to how
> gtests behave.
>
> -- adrian
>
> The way this is handled in LLVM is that if you think tests are
> sufficiently different that they could be broken by different types of
> changes, you split them up into different files.
>
> Do you feel LLDB is fundamentally different in this regard?
>
> On Wed, Apr 25, 2018 at 8:27 AM Adrian Prantl  wrote:
>
>> Disregarding the added complexity I believe that being able to
>> communicate the number of tests filed/passed inside a single file would be
>> very valuable. For example when one test gets broken by upstream clang
>> changes and it takes a few days to fix it properly, we don't want to loose
>> signal on the other tests in the same file during that period. I think it's
>> worth it.
>>
>> -- adrian
>>
>>
>> On Apr 25, 2018, at 8:13 AM, Zachary Turner  wrote:
>>
>> Well let’s see what Davide and Adrian think. I’m more of an outsider
>> these days so consider my perspective an llvm-centric one, which would
>> sometimes (but not always) be the best for lldb
>> On Wed, Apr 25, 2018 at 12:31 AM Pavel Labath via Phabricator <
>> revi...@reviews.llvm.org> wrote:
>>
>>> labath added a comment.
>>>
>>> In https://reviews.llvm.org/D46005#1077016, @zturner wrote:
>>>
>>> > Note that there's currently no precedent (that i'm aware of anwyay) in
>>> LLVM or any of its subprojects for splitting the running of a single file
>>> into multiple parallel jobs.  All of LLVM's other projects parallelize at
>>> file granularity, and so it's up to to the person writing tests to ensure
>>> that the file granularity matches that with which they want tests to be
>>> parallelized at.
>>>
>>>
>>> There's always gtest. Regardless of whether you consider a .cpp file or
>>> the built exectable to be a "test file", it will generally contain a number
>>> of tests. And arguably, our test suite is more similar to the gtest suite
>>> than the traditional lit (ShTest) suites (for one, it hijacks a third party
>>> unit testing library, and then tries to make it run under lit). And we run
>>> all gtest tests individually (I've often wondered what kind of performance
>>> impact that has, but they seem to be running fast enough anyway...).
>>>
>>> For what it's worth, paralelization is not my motivation here. There
>>> some tests which run disproportionately long, and this will speed them up,
>>> but that may be offset by the fact we need to start more work this way (if
>>> anyone is interested I can try to do some measurements). My main reason for
>>> doing this is so that we can have better mapping of test result states. As
>>> it stands now, we only have two possible results for a test: passed or
>>> failed. Lit has more results than that, and they roughly match the existing
>>> dotest states (the different treatment of XFAILs is the biggest
>>> difference), so it should be possible to represent them with more fidelity.
>>> However, right now it's not possible to translate them reasonably, as a
>>> "single test" can result in any number of fails/xfails/skips/... (or no
>>> results at all).
>>>
>>> > That doesn't mean it's not possible (as you've shown here), but it
>>> adds some additional complexity and I'm not sure it's worth it.
>>>
>>> It adds complexity, but not much imho. I was actually pleasantly
>>> surprised at how easy it was to pull this off. The entire implementation is
>>> 78 LOC right now. The changes to the test format will probably push it over
>>> a 100, but not by much.
>>>
>>> That said, I am not interested in forcing this onto everyone. I did it
>>> because it seemed like a nice thing to have and I was curious if it is
>>> doable (easily). However, if there's no interest for it, then I can just
>>> abandon it..
>>>
>>>
>>> https://reviews.llvm.org/D46005
>>>
>>>
>>>
>>>
>>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.

[Greg Clayton via lldb-commits]

> On Apr 25, 2018, at 12:54 AM, Pavel Labath via Phabricator 
>  wrote:
> 
> labath added a comment.
> 
> This code itself looks fine, I have just two minor comments.
> 
> However, I do have a question about performance. I remember us being very 
> worried about performance in the past, so we ended up putting in this like 
> r298876. This removes the normalization step during FileSpec comparison, but 
> it introduces mandatory normalization upon every FileSpec creation, so it's 
> not obvious to me what will this do to performance. Should we try to 
> benchmark this somehow?

I will try debugging LLDB with clang and llvm with debug info and benchmark 
setting setting a file and line breakpoint using a base name to see if there is 
a regression. This will cause all line tables to be created for all compile 
units and all compile unit files in the DWARF line table. I'll let you know 
what I found.
> 
> Jim, you seem to have encountered a case when the normalization was a 
> bottleneck (r298876). Do you remember what situation was that in?

r298876 was for making sure two paths compare correctly and each time you set a 
breakpoint it would normalize off the trailing ".". We will do this once now 
and of course this code form this patch is removed since the normalization will 
take care of it. Setting breakpoints should be faster if I had to guess since 
we are removing the need to check for and normalize a path if it is relative.
> 
> 
> 
> Comment at: source/Utility/FileSpec.cpp:245
> 
> +  llvm::StringRef resolve_path_ref2(resolved.c_str());
> +
> 
> It looks like this is unused.
> 
> 
> 
> Comment at: source/Utility/FileSpec.cpp:269-270
> +  else {
> +// Make sure we don't end up with "." in both the directory and filename.
> +// If we do, clear the directory. 
> +m_filename.SetString(".");
> 
> `remove_dots` should never produce a path like this, so we should be able to 
> revert this now.
> 
> 
> https://reviews.llvm.org/D45977
> 
> 
> 

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

Re: [Lldb-commits] [lldb] r330708 - [dotest] Make the set of tests independent of the test configuration

[Vedant Kumar via lldb-commits]

Hi Pavel,

> On Apr 24, 2018, at 3:51 AM, Pavel Labath via lldb-commits 
>  wrote:
> 
> Author: labath
> Date: Tue Apr 24 03:51:44 2018
> New Revision: 330708
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=330708&view=rev
> Log:
> [dotest] Make the set of tests independent of the test configuration
> 
> Summary:
> In the magic test duplicator, we were making the decision whether to
> create a test variant based on the compiler and the target platform.
> This meant that the set of known tests was different for each test
> configuration.
> 
> This patch makes the set of generated test variants static and handles
> the skipping via runtime checks instead. This is more consistent with
> how we do other test-skipping decision (e.g. for libc++ tests), and
> makes it easier to expose the full set of tests to lit, which now does
> not need to know anything about what things can potentially cause tests
> to appear or disappear.
> 
> Reviewers: JDevlieghere, aprantl
> 
> Subscribers: eraman, lldb-commits
> 
> Differential Revision: https://reviews.llvm.org/D45949
> 
> Modified:
>lldb/trunk/packages/Python/lldbsuite/test/dotest.py
>lldb/trunk/packages/Python/lldbsuite/test/lldbinline.py
>lldb/trunk/packages/Python/lldbsuite/test/lldbtest.py
> 
> Modified: lldb/trunk/packages/Python/lldbsuite/test/dotest.py
> URL: 
> http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/dotest.py?rev=330708&r1=330707&r2=330708&view=diff
> ==
> --- lldb/trunk/packages/Python/lldbsuite/test/dotest.py (original)
> +++ lldb/trunk/packages/Python/lldbsuite/test/dotest.py Tue Apr 24 03:51:44 
> 2018
> @@ -1104,6 +1104,22 @@ def checkLibcxxSupport():
> print("Libc++ tests will not be run because: " + reason)
> configuration.skipCategories.append("libc++")
> 
> +def checkDebugInfoSupport():
> +import lldb
> +
> +platform = lldb.DBG.GetSelectedPlatform().GetTriple().split('-')[2]
> +compiler = configuration.compiler
> +skipped = []
> +for cat in test_categories.debug_info_categories:
> +if cat in configuration.categoriesList:
> +continue # Category explicitly requested, let it run.

Is there a missing check here for:

if cat in configuration.skipCategories:
  skip(cat)

Without this, I'm not sure how 'dotest --skip-category days' would work on 
Darwin.

> +if test_categories.is_supported_on_platform(cat, platform, compiler):
> +continue
> +configuration.skipCategories.append(cat)
> +skipped.append(cat)
> +if skipped:
> +print("Skipping following debug info categories:", skipped)
> +
> def run_suite():
> # On MacOS X, check to make sure that domain for com.apple.DebugSymbols 
> defaults
> # does not exist before proceeding to running the test suite.
> @@ -1212,6 +1228,7 @@ def run_suite():
> target_platform = lldb.DBG.GetSelectedPlatform().GetTriple().split('-')[2]
> 
> checkLibcxxSupport()
> +checkDebugInfoSupport()
> 
> # Don't do debugserver tests on everything except OS X.
> configuration.dont_do_debugserver_test = "linux" in target_platform or 
> "freebsd" in target_platform or "windows" in target_platform
> 
> Modified: lldb/trunk/packages/Python/lldbsuite/test/lldbinline.py
> URL: 
> http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/lldbinline.py?rev=330708&r1=330707&r2=330708&view=diff
> ==
> --- lldb/trunk/packages/Python/lldbsuite/test/lldbinline.py (original)
> +++ lldb/trunk/packages/Python/lldbsuite/test/lldbinline.py Tue Apr 24 
> 03:51:44 2018
> @@ -241,23 +241,14 @@ def MakeInlineTest(__file, __globals, de
> test = type(test_name, (InlineTest,), {'using_dsym': None})
> test.name = test_name
> 
> -target_platform = 
> lldb.DBG.GetSelectedPlatform().GetTriple().split('-')[2]
> -if test_categories.is_supported_on_platform(
> -"dsym", target_platform, configuration.compiler):
> -test.test_with_dsym = ApplyDecoratorsToFunction(
> -test._InlineTest__test_with_dsym, decorators)
> -if test_categories.is_supported_on_platform(
> -"dwarf", target_platform, configuration.compiler):
> -test.test_with_dwarf = ApplyDecoratorsToFunction(
> -test._InlineTest__test_with_dwarf, decorators)
> -if test_categories.is_supported_on_platform(
> -"dwo", target_platform, configuration.compiler):
> -test.test_with_dwo = ApplyDecoratorsToFunction(
> -test._InlineTest__test_with_dwo, decorators)
> -if test_categories.is_supported_on_platform(
> -"gmodules", target_platform, configuration.compiler):
> -test.test_with_gmodules = ApplyDecoratorsToFunction(
> -test._InlineTest__test_with_gmodules, decorators)
> +test.test_with_dsym = ApplyDecoratorsToFunction(
> +

Re: [Lldb-commits] [lldb] r330708 - [dotest] Make the set of tests independent of the test configuration

[Vedant Kumar via lldb-commits]

> On Apr 25, 2018, at 11:07 AM, Vedant Kumar  wrote:
> 
> Hi Pavel,
> 
>> On Apr 24, 2018, at 3:51 AM, Pavel Labath via lldb-commits 
>>  wrote:
>> 
>> Author: labath
>> Date: Tue Apr 24 03:51:44 2018
>> New Revision: 330708
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=330708&view=rev
>> Log:
>> [dotest] Make the set of tests independent of the test configuration
>> 
>> Summary:
>> In the magic test duplicator, we were making the decision whether to
>> create a test variant based on the compiler and the target platform.
>> This meant that the set of known tests was different for each test
>> configuration.
>> 
>> This patch makes the set of generated test variants static and handles
>> the skipping via runtime checks instead. This is more consistent with
>> how we do other test-skipping decision (e.g. for libc++ tests), and
>> makes it easier to expose the full set of tests to lit, which now does
>> not need to know anything about what things can potentially cause tests
>> to appear or disappear.
>> 
>> Reviewers: JDevlieghere, aprantl
>> 
>> Subscribers: eraman, lldb-commits
>> 
>> Differential Revision: https://reviews.llvm.org/D45949
>> 
>> Modified:
>>   lldb/trunk/packages/Python/lldbsuite/test/dotest.py
>>   lldb/trunk/packages/Python/lldbsuite/test/lldbinline.py
>>   lldb/trunk/packages/Python/lldbsuite/test/lldbtest.py
>> 
>> Modified: lldb/trunk/packages/Python/lldbsuite/test/dotest.py
>> URL: 
>> http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/dotest.py?rev=330708&r1=330707&r2=330708&view=diff
>> ==
>> --- lldb/trunk/packages/Python/lldbsuite/test/dotest.py (original)
>> +++ lldb/trunk/packages/Python/lldbsuite/test/dotest.py Tue Apr 24 03:51:44 
>> 2018
>> @@ -1104,6 +1104,22 @@ def checkLibcxxSupport():
>>print("Libc++ tests will not be run because: " + reason)
>>configuration.skipCategories.append("libc++")
>> 
>> +def checkDebugInfoSupport():
>> +import lldb
>> +
>> +platform = lldb.DBG.GetSelectedPlatform().GetTriple().split('-')[2]
>> +compiler = configuration.compiler
>> +skipped = []
>> +for cat in test_categories.debug_info_categories:
>> +if cat in configuration.categoriesList:
>> +continue # Category explicitly requested, let it run.
> 
> Is there a missing check here for:
> 
> if cat in configuration.skipCategories:
>  skip(cat)
> 
> Without this, I'm not sure how 'dotest --skip-category days' would work on 
> Darwin.

Sorry, I meant to type: --skip-category dsym.

vedant

> 
>> +if test_categories.is_supported_on_platform(cat, platform, 
>> compiler):
>> +continue
>> +configuration.skipCategories.append(cat)
>> +skipped.append(cat)
>> +if skipped:
>> +print("Skipping following debug info categories:", skipped)
>> +
>> def run_suite():
>># On MacOS X, check to make sure that domain for com.apple.DebugSymbols 
>> defaults
>># does not exist before proceeding to running the test suite.
>> @@ -1212,6 +1228,7 @@ def run_suite():
>>target_platform = lldb.DBG.GetSelectedPlatform().GetTriple().split('-')[2]
>> 
>>checkLibcxxSupport()
>> +checkDebugInfoSupport()
>> 
>># Don't do debugserver tests on everything except OS X.
>>configuration.dont_do_debugserver_test = "linux" in target_platform or 
>> "freebsd" in target_platform or "windows" in target_platform
>> 
>> Modified: lldb/trunk/packages/Python/lldbsuite/test/lldbinline.py
>> URL: 
>> http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/lldbinline.py?rev=330708&r1=330707&r2=330708&view=diff
>> ==
>> --- lldb/trunk/packages/Python/lldbsuite/test/lldbinline.py (original)
>> +++ lldb/trunk/packages/Python/lldbsuite/test/lldbinline.py Tue Apr 24 
>> 03:51:44 2018
>> @@ -241,23 +241,14 @@ def MakeInlineTest(__file, __globals, de
>>test = type(test_name, (InlineTest,), {'using_dsym': None})
>>test.name = test_name
>> 
>> -target_platform = 
>> lldb.DBG.GetSelectedPlatform().GetTriple().split('-')[2]
>> -if test_categories.is_supported_on_platform(
>> -"dsym", target_platform, configuration.compiler):
>> -test.test_with_dsym = ApplyDecoratorsToFunction(
>> -test._InlineTest__test_with_dsym, decorators)
>> -if test_categories.is_supported_on_platform(
>> -"dwarf", target_platform, configuration.compiler):
>> -test.test_with_dwarf = ApplyDecoratorsToFunction(
>> -test._InlineTest__test_with_dwarf, decorators)
>> -if test_categories.is_supported_on_platform(
>> -"dwo", target_platform, configuration.compiler):
>> -test.test_with_dwo = ApplyDecoratorsToFunction(
>> -test._InlineTest__test_with_dwo, decorators)
>> -if test_categories.is_supported_on_platform(
>> -"gmodules", target

[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In https://reviews.llvm.org/D45977#1077719, @labath wrote:

> This code itself looks fine, I have just two minor comments.
>
> However, I do have a question about performance. I remember us being very 
> worried about performance in the past, so we ended up putting in this like 
> r298876. This removes the normalization step during FileSpec comparison, but 
> it introduces mandatory normalization upon every FileSpec creation, so it's 
> not obvious to me what will this do to performance. Should we try to 
> benchmark this somehow?


Ok, so I ran a benchmark and we are about 7% slower on a completely cold file 
cache, and 9% slower on a warm file cache. If I add the needsNormalization() 
function back in, we are 7% faster for cold, and 10% faster for warm. So I will 
add the needsNormalization() back in.


https://reviews.llvm.org/D45977



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

Re: [Lldb-commits] [lldb] r330708 - [dotest] Make the set of tests independent of the test configuration

[Pavel Labath via lldb-commits]

On Wed, 25 Apr 2018 at 19:07, Vedant Kumar  wrote:

> Hi Pavel,

> > On Apr 24, 2018, at 3:51 AM, Pavel Labath via lldb-commits <
lldb-commits@lists.llvm.org> wrote:
> > +def checkDebugInfoSupport():
> > +import lldb
> > +
> > +platform = lldb.DBG.GetSelectedPlatform().GetTriple().split('-')[2]
> > +compiler = configuration.compiler
> > +skipped = []
> > +for cat in test_categories.debug_info_categories:
> > +if cat in configuration.categoriesList:
> > +continue # Category explicitly requested, let it run.

> Is there a missing check here for:

> if cat in configuration.skipCategories:
>skip(cat)
I don't see why that would be needed. If you pass --skip-categories dsym,
the dsym category will already be present in "configuration.skipCategories"
so there's nothing to do here.

What this code does is it extends the skipped categories list based on the
supported configurations. So, if you're on darwin it should behave as if
you typed --skip-cateogory "dwo" on the command line. The actual code which
does category-based skipping lives elsewhere, I don't remember the exact
file off the top of my head.

> > @@ -1732,14 +1732,11 @@ class LLDBTestCaseFactory(type):
> > for attrname, attrvalue in attrs.items():
> > if attrname.startswith("test") and not getattr(
> > attrvalue, "__no_debug_info_test__", False):
> > -target_platform = lldb.DBG.GetSelectedPlatform(
> > -).GetTriple().split('-')[2]
> >
> > # If any debug info categories were explicitly tagged,
assume that list to be
> > # authoritative.  If none were specified, try with all
debug
> > # info formats.
> > -all_dbginfo_categories = set(
> > -test_categories.debug_info_categories) -
set(configuration.skipCategories)
> > +all_dbginfo_categories =
set(test_categories.debug_info_categories)

> Ditto. I'm not sure why it shouldn't be possible to skip debug info
categories.

It should still be possible to skip based on debug info categories. The
only difference is that before this patch passing say --skip-category dwo
would cause the dwo tests to not exist, whereas now they will still appear
in the test list (but they will be skipped).

This is the same way we use for auto-skipping libc++/libstdc++ support for
data formatter tests, and it works fine there. Did you actually check that
it is not possible to skip dsym tests?
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D40475: DWZ 06/07: DWZ test mode

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil updated this revision to Diff 143992.

https://reviews.llvm.org/D40475

Files:
  packages/Python/lldbsuite/test/dotest.py
  packages/Python/lldbsuite/test/lldbinline.py
  packages/Python/lldbsuite/test/lldbtest.py
  packages/Python/lldbsuite/test/make/Makefile.rules
  packages/Python/lldbsuite/test/plugins/builder_base.py
  packages/Python/lldbsuite/test/test_categories.py
  unittests/SymbolFile/CMakeLists.txt
  unittests/SymbolFile/DWZ/CMakeLists.txt
  unittests/SymbolFile/DWZ/Inputs/dwztest.c
  unittests/SymbolFile/DWZ/Inputs/dwztest.debug
  unittests/SymbolFile/DWZ/Inputs/dwztest.debug.dwz
  unittests/SymbolFile/DWZ/Inputs/dwztest.out
  unittests/SymbolFile/DWZ/SymbolFileDWZTests.cpp

Index: unittests/SymbolFile/DWZ/SymbolFileDWZTests.cpp
===
--- /dev/null
+++ unittests/SymbolFile/DWZ/SymbolFileDWZTests.cpp
@@ -0,0 +1,89 @@
+//===-- SymbolFileDWZTests.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "TestingSupport/TestUtilities.h"
+
+#include "Plugins/SymbolFile/DWARF/SymbolFileDWARF.h"
+#include "Plugins/ObjectFile/ELF/ObjectFileELF.h"
+#include "Plugins/SymbolVendor/ELF/SymbolVendorELF.h"
+#include "lldb/Core/Module.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Symbol/ClangASTContext.h"
+#include "lldb/Symbol/SymbolVendor.h"
+#include "lldb/Utility/ArchSpec.h"
+#include "lldb/Utility/FileSpec.h"
+
+#if defined(_MSC_VER)
+#include "lldb/Host/windows/windows.h"
+#include 
+#endif
+
+#include 
+
+using namespace lldb_private;
+
+class SymbolFileDWZTests : public testing::Test {
+public:
+  void SetUp() override {
+// Initialize and TearDown the plugin every time, so we get a brand new
+// AST every time so that modifications to the AST from each test don't
+// leak into the next test.
+#if defined(_MSC_VER)
+::CoInitializeEx(nullptr, COINIT_MULTITHREADED);
+#endif
+
+HostInfo::Initialize();
+SymbolFileDWARF::Initialize();
+ClangASTContext::Initialize();
+ObjectFileELF::Initialize();
+SymbolVendorELF::Initialize();
+
+m_dwztest_out = GetInputFilePath("dwztest.out");
+  }
+
+  void TearDown() override {
+SymbolVendorELF::Terminate();
+ObjectFileELF::Terminate();
+ClangASTContext::Terminate();
+SymbolFileDWARF::Terminate();
+HostInfo::Terminate();
+
+#if defined(_MSC_VER)
+::CoUninitialize();
+#endif
+  }
+
+protected:
+  std::string m_dwztest_out;
+};
+
+TEST_F(SymbolFileDWZTests, TestSimpleClassTypes) {
+  FileSpec fspec(m_dwztest_out.c_str(), false);
+  ArchSpec aspec("x86_64-pc-linux");
+  lldb::ModuleSP module = std::make_shared(fspec, aspec);
+
+  SymbolVendor *plugin = module->GetSymbolVendor();
+  EXPECT_NE(nullptr, plugin);
+  SymbolFile *symfile = plugin->GetSymbolFile();
+  EXPECT_NE(nullptr, symfile);
+  EXPECT_EQ(symfile->GetPluginName(), SymbolFileDWARF::GetPluginNameStatic());
+  SymbolContext sc;
+  llvm::DenseSet searched_files;
+  TypeMap results;
+  EXPECT_EQ(1u,
+  symfile->FindTypes(sc, ConstString("StructMovedToDWZCommonFile"), nullptr,
+  false, 0, searched_files, results));
+  EXPECT_EQ(1u, results.GetSize());
+  lldb::TypeSP udt_type = results.GetTypeAtIndex(0);
+  EXPECT_EQ(ConstString("StructMovedToDWZCommonFile"), udt_type->GetName());
+  CompilerType compiler_type = udt_type->GetForwardCompilerType();
+  EXPECT_TRUE(ClangASTContext::IsClassType(compiler_type.GetOpaqueQualType()));
+}
Index: unittests/SymbolFile/DWZ/Inputs/dwztest.c
===
--- /dev/null
+++ unittests/SymbolFile/DWZ/Inputs/dwztest.c
@@ -0,0 +1,9 @@
+// gcc -Wall -g -o dwztest.out dwztest.c; eu-strip --remove-comment -f dwztest.debug dwztest.out; cp -p dwztest.debug dwztest.debug.dup; dwz -m dwztest.debug.dwz dwztest.debug dwztest.debug.dup;rm dwztest.debug.dup; /usr/lib/rpm/sepdebugcrcfix . dwztest.out 
+
+struct StructMovedToDWZCommonFile {
+  int i1, i2, i3, i4, i5, i6, i7, i8, i9;
+} VarWithStructMovedToDWZCommonFile;
+static const int sizeof_StructMovedToDWZCommonFile = sizeof(struct StructMovedToDWZCommonFile);
+int main() {
+  return sizeof_StructMovedToDWZCommonFile;
+}
Index: unittests/SymbolFile/DWZ/CMakeLists.txt
===
--- /dev/null
+++ unittests/SymbolFile/DWZ/CMakeLists.txt
@@ -0,0 +1,21 @@
+add_lldb_unittest(SymbolFileDWZTests
+  SymbolFileDWZTests.cpp
+
+  LINK_LIBS
+lldbCore
+lldbHost
+lldbSymbol
+lldbPluginSymbolFileDWARF
+lldbUtilityHelpers
+lldbPluginObjectFileELF
+lldbPluginSymbolVendorELF
+  LINK_COMPONENTS
+Support
+  )
+
+set(test_inputs
+   dwztest.out
+   dwztest.debug
+   dwztest.debug

Re: [Lldb-commits] [lldb] r330708 - [dotest] Make the set of tests independent of the test configuration

[Vedant Kumar via lldb-commits]

> On Apr 25, 2018, at 12:04 PM, Pavel Labath  wrote:
> 
> On Wed, 25 Apr 2018 at 19:07, Vedant Kumar  wrote:
> 
>> Hi Pavel,
> 
>>> On Apr 24, 2018, at 3:51 AM, Pavel Labath via lldb-commits <
> lldb-commits@lists.llvm.org> wrote:
>>> +def checkDebugInfoSupport():
>>> +import lldb
>>> +
>>> +platform = lldb.DBG.GetSelectedPlatform().GetTriple().split('-')[2]
>>> +compiler = configuration.compiler
>>> +skipped = []
>>> +for cat in test_categories.debug_info_categories:
>>> +if cat in configuration.categoriesList:
>>> +continue # Category explicitly requested, let it run.
> 
>> Is there a missing check here for:
> 
>> if cat in configuration.skipCategories:
>>   skip(cat)
> I don't see why that would be needed. If you pass --skip-categories dsym,
> the dsym category will already be present in "configuration.skipCategories"
> so there's nothing to do here.
> 
> What this code does is it extends the skipped categories list based on the
> supported configurations. So, if you're on darwin it should behave as if
> you typed --skip-cateogory "dwo" on the command line. The actual code which
> does category-based skipping lives elsewhere, I don't remember the exact
> file off the top of my head.
> 
>>> @@ -1732,14 +1732,11 @@ class LLDBTestCaseFactory(type):
>>>for attrname, attrvalue in attrs.items():
>>>if attrname.startswith("test") and not getattr(
>>>attrvalue, "__no_debug_info_test__", False):
>>> -target_platform = lldb.DBG.GetSelectedPlatform(
>>> -).GetTriple().split('-')[2]
>>> 
>>># If any debug info categories were explicitly tagged,
> assume that list to be
>>># authoritative.  If none were specified, try with all
> debug
>>># info formats.
>>> -all_dbginfo_categories = set(
>>> -test_categories.debug_info_categories) -
> set(configuration.skipCategories)
>>> +all_dbginfo_categories =
> set(test_categories.debug_info_categories)
> 
>> Ditto. I'm not sure why it shouldn't be possible to skip debug info
> categories.
> 
> It should still be possible to skip based on debug info categories. The
> only difference is that before this patch passing say --skip-category dwo
> would cause the dwo tests to not exist, whereas now they will still appear
> in the test list (but they will be skipped).

Thanks for explaining.


> This is the same way we use for auto-skipping libc++/libstdc++ support for
> data formatter tests, and it works fine there. Did you actually check that
> it is not possible to skip dsym tests?

Sorry, I hadn't checked. I was confused about how the category skipping worked 
at runtime. It is still possible to skip inline dsym tests.

thanks,
vedant
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D46083: Move the persistent variable counter into Target

[Adrian Prantl via Phabricator via lldb-commits]

aprantl created this revision.
aprantl added a reviewer: jingham.

Move the persistent variable counter into Target so it can be shared across 
multiple language plugins.

  

In a multi-language project it is counterintuitive to have a result
variables reuse numbers just because they are using a different
language plugin in LLDB (but not for example, when they are
Objective-C versus C++, since they are both handled by Clang).

  

This is NFC on llvm.org except for the Go plugin.

  

rdar://problem/39299889


https://reviews.llvm.org/D46083

Files:
  include/lldb/Expression/ExpressionVariable.h
  include/lldb/Target/Target.h
  source/Core/ValueObject.cpp
  source/Expression/Materializer.cpp
  source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp
  source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h
  source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
  source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
  source/Plugins/ExpressionParser/Go/GoUserExpression.cpp
  source/Plugins/ExpressionParser/Go/GoUserExpression.h
  source/Target/ABI.cpp

Index: source/Target/ABI.cpp
===
--- source/Target/ABI.cpp
+++ source/Target/ABI.cpp
@@ -102,15 +102,16 @@
   // work.
 
   if (persistent) {
+Target &target = *thread.CalculateTarget();
 PersistentExpressionState *persistent_expression_state =
-thread.CalculateTarget()->GetPersistentExpressionStateForLanguage(
+target.GetPersistentExpressionStateForLanguage(
 ast_type.GetMinimumLanguage());
 
 if (!persistent_expression_state)
   return ValueObjectSP();
 
 ConstString persistent_variable_name(
-persistent_expression_state->GetNextPersistentVariableName());
+persistent_expression_state->GetNextPersistentVariableName(target));
 
 lldb::ValueObjectSP const_valobj_sp;
 
Index: source/Plugins/ExpressionParser/Go/GoUserExpression.h
===
--- source/Plugins/ExpressionParser/Go/GoUserExpression.h
+++ source/Plugins/ExpressionParser/Go/GoUserExpression.h
@@ -29,7 +29,7 @@
 public:
   GoPersistentExpressionState();
 
-  ConstString GetNextPersistentVariableName() override;
+  ConstString GetNextPersistentVariableName(Target &target) override;
 
   void RemovePersistentVariable(lldb::ExpressionVariableSP variable) override;
 
Index: source/Plugins/ExpressionParser/Go/GoUserExpression.cpp
===
--- source/Plugins/ExpressionParser/Go/GoUserExpression.cpp
+++ source/Plugins/ExpressionParser/Go/GoUserExpression.cpp
@@ -272,7 +272,7 @@
   PersistentExpressionState *pv =
   target->GetPersistentExpressionStateForLanguage(eLanguageTypeGo);
   if (pv != nullptr) {
-result->SetName(pv->GetNextPersistentVariableName());
+result->SetName(pv->GetNextPersistentVariableName(*target));
 pv->AddVariable(result);
   }
   return lldb::eExpressionCompleted;
@@ -651,11 +651,12 @@
 GoPersistentExpressionState::GoPersistentExpressionState()
 : PersistentExpressionState(eKindGo) {}
 
-ConstString GoPersistentExpressionState::GetNextPersistentVariableName() {
+ConstString
+GoPersistentExpressionState::GetNextPersistentVariableName(Target &target) {
   char name_cstr[256];
   // We can't use the same variable format as clang.
   ::snprintf(name_cstr, sizeof(name_cstr), "$go%u",
- m_next_persistent_variable_id++);
+ target.GetNextPersistentVariableIndex());
   ConstString name(name_cstr);
   return name;
 }
Index: source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
===
--- source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
+++ source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
@@ -178,7 +178,7 @@
 
   class ResultDelegate : public Materializer::PersistentVariableDelegate {
   public:
-ResultDelegate();
+ResultDelegate(lldb::TargetSP target) : m_target_sp(target) {}
 ConstString GetName() override;
 void DidDematerialize(lldb::ExpressionVariableSP &variable) override;
 
@@ -188,6 +188,7 @@
   private:
 PersistentExpressionState *m_persistent_state;
 lldb::ExpressionVariableSP m_variable;
+lldb::TargetSP m_target_sp;
   };
 
   ResultDelegate m_result_delegate;
Index: source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
===
--- source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
+++ source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
@@ -65,7 +65,8 @@
  options),
   m_type_system_helper(*m_target_wp.lock().get(),
options.GetExecutionPolicy() ==
-   eExecutionPolicyTopLevel) {
+   eExecutionPolicyTopLevel),
+  m_result_delegate(exe_scop

[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.

[Greg Clayton via Phabricator via lldb-commits]

clayborg updated this revision to Diff 144021.
clayborg added a comment.

After doing performance tests, the code was 7 to 10 % slower if we didn't check 
if a path needs normalization due to the llvm code making arrays of StringRef 
objects and appending a path together. Restored and even improved performance 
after adding back the needsNormalization() function that quickly checks if a 
path needs normalization and avoids the splitting of the path and 
reconstruction of the path if it isn't needed.


https://reviews.llvm.org/D45977

Files:
  include/lldb/Core/FileSpecList.h
  include/lldb/Utility/FileSpec.h
  source/Breakpoint/BreakpointResolverFileLine.cpp
  source/Core/FileSpecList.cpp
  source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp
  source/Plugins/SymbolFile/DWARF/DWARFDebugLine.h
  source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  source/Symbol/CompileUnit.cpp
  source/Symbol/Declaration.cpp
  source/Utility/FileSpec.cpp
  unittests/Utility/FileSpecTest.cpp

Index: unittests/Utility/FileSpecTest.cpp
===
--- unittests/Utility/FileSpecTest.cpp
+++ unittests/Utility/FileSpecTest.cpp
@@ -54,17 +54,14 @@
 
   FileSpec fs_posix_trailing_slash("/foo/bar/", false,
FileSpec::ePathSyntaxPosix);
-  EXPECT_STREQ("/foo/bar/.", fs_posix_trailing_slash.GetCString());
-  EXPECT_STREQ("/foo/bar", fs_posix_trailing_slash.GetDirectory().GetCString());
-  EXPECT_STREQ(".", fs_posix_trailing_slash.GetFilename().GetCString());
+  EXPECT_STREQ("/foo/bar", fs_posix_trailing_slash.GetCString());
+  EXPECT_STREQ("/foo", fs_posix_trailing_slash.GetDirectory().GetCString());
+  EXPECT_STREQ("bar", fs_posix_trailing_slash.GetFilename().GetCString());
 
   FileSpec fs_windows_trailing_slash("F:\\bar\\", false,
  FileSpec::ePathSyntaxWindows);
-  EXPECT_STREQ("F:\\bar\\.", fs_windows_trailing_slash.GetCString());
-  // EXPECT_STREQ("F:\\bar",
-  // fs_windows_trailing_slash.GetDirectory().GetCString()); // It returns
-  // "F:/bar"
-  EXPECT_STREQ(".", fs_windows_trailing_slash.GetFilename().GetCString());
+  EXPECT_STREQ("F:\\bar", fs_windows_trailing_slash.GetCString());
+  EXPECT_STREQ("bar", fs_windows_trailing_slash.GetFilename().GetCString());
 }
 
 TEST(FileSpecTest, AppendPathComponent) {
@@ -131,32 +128,13 @@
   EXPECT_STREQ("F:\\bar", fs_windows_root.GetCString());
 }
 
-static void Compare(const FileSpec &one, const FileSpec &two, bool full_match,
-bool remove_backup_dots, bool result) {
-  EXPECT_EQ(result, FileSpec::Equal(one, two, full_match, remove_backup_dots))
-  << "File one: " << one.GetCString() << "\nFile two: " << two.GetCString()
-  << "\nFull match: " << full_match
-  << "\nRemove backup dots: " << remove_backup_dots;
-}
-
 TEST(FileSpecTest, EqualSeparator) {
   FileSpec backward("C:\\foo\\bar", false, FileSpec::ePathSyntaxWindows);
   FileSpec forward("C:/foo/bar", false, FileSpec::ePathSyntaxWindows);
   EXPECT_EQ(forward, backward);
-
-  const bool full_match = true;
-  const bool remove_backup_dots = true;
-  const bool match = true;
-  Compare(forward, backward, full_match, remove_backup_dots, match);
-  Compare(forward, backward, full_match, !remove_backup_dots, match);
-  Compare(forward, backward, !full_match, remove_backup_dots, match);
-  Compare(forward, backward, !full_match, !remove_backup_dots, match);
 }
 
 TEST(FileSpecTest, EqualDotsWindows) {
-  const bool full_match = true;
-  const bool remove_backup_dots = true;
-  const bool match = true;
   std::pair tests[] = {
   {R"(C:\foo\bar\baz)", R"(C:\foo\foo\..\bar\baz)"},
   {R"(C:\bar\baz)", R"(C:\foo\..\bar\baz)"},
@@ -170,18 +148,11 @@
   for (const auto &test : tests) {
 FileSpec one(test.first, false, FileSpec::ePathSyntaxWindows);
 FileSpec two(test.second, false, FileSpec::ePathSyntaxWindows);
-EXPECT_NE(one, two);
-Compare(one, two, full_match, remove_backup_dots, match);
-Compare(one, two, full_match, !remove_backup_dots, !match);
-Compare(one, two, !full_match, remove_backup_dots, match);
-Compare(one, two, !full_match, !remove_backup_dots, !match);
+EXPECT_EQ(one, two);
   }
 }
 
 TEST(FileSpecTest, EqualDotsPosix) {
-  const bool full_match = true;
-  const bool remove_backup_dots = true;
-  const bool match = true;
   std::pair tests[] = {
   {R"(/foo/bar/baz)", R"(/foo/foo/../bar/baz)"},
   {R"(/bar/baz)", R"(/foo/../bar/baz)"},
@@ -193,18 +164,11 @@
   for (const auto &test : tests) {
 FileSpec one(test.first, false, FileSpec::ePathSyntaxPosix);
 FileSpec two(test.second, false, FileSpec::ePathSyntaxPosix);
-EXPECT_NE(one, two);
-Compare(one, two, full_match, remove_backup_dots, match);
-Compare(one, two, full_match, !remove_backup_dots, !match);
-Compare(one, two, !full_match, remove_backup_dots, match);
-Compare(one, two,

[Lldb-commits] [lldb] r330877 - [debugserver] Return 'ios' instead of 'iphoneos' for the ostype.

[Frederic Riss via lldb-commits]

Author: friss
Date: Wed Apr 25 15:12:12 2018
New Revision: 330877

URL: http://llvm.org/viewvc/llvm-project?rev=330877&view=rev
Log:
[debugserver] Return 'ios' instead of 'iphoneos' for the ostype.

When I merged the 2 codepaths that return an OS type, I hade
checked that the places accepting 'iphoneos' would also accept
'ios', but then I got it backwards and return 'iphoneos'.

We use this value to build triples, and there 'iphoneos' is
invalid.

This also makes the test slightly simpler.

Modified:

lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-server/TestAppleSimulatorOSType.py
lldb/trunk/tools/debugserver/source/MacOSX/MachProcess.mm

Modified: 
lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-server/TestAppleSimulatorOSType.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-server/TestAppleSimulatorOSType.py?rev=330877&r1=330876&r2=330877&view=diff
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-server/TestAppleSimulatorOSType.py
 (original)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-server/TestAppleSimulatorOSType.py
 Wed Apr 25 15:12:12 2018
@@ -13,14 +13,14 @@ class TestAppleSimulatorOSType(gdbremote
 
 mydir = TestBase.compute_mydir(__file__)
 
-def check_simulator_ostype(self, sdk, platform_names, arch='x86_64'):
+def check_simulator_ostype(self, sdk, platform, arch='x86_64'):
 sim_devices_str = subprocess.check_output(['xcrun', 'simctl', 'list',
'-j', 'devices'])
 sim_devices = json.loads(sim_devices_str)['devices']
 # Find an available simulator for the requested platform
 deviceUDID = None
 for (runtime,devices) in sim_devices.items():
-if not any(p in runtime.lower() for p in platform_names):
+if not platform in runtime.lower():
 continue
 for device in devices:
 if device['availability'] != '(available)':
@@ -32,7 +32,7 @@ class TestAppleSimulatorOSType(gdbremote
 
 # Launch the process using simctl
 self.assertIsNotNone(deviceUDID)
-exe_name = 'test_simulator_platform_{}'.format(platform_names[0])
+exe_name = 'test_simulator_platform_{}'.format(platform)
 sdkroot = subprocess.check_output(['xcrun', '--show-sdk-path', '--sdk',
sdk])
 self.build(dictionary={ 'EXE': exe_name, 'SDKROOT': sdkroot.strip(),
@@ -75,7 +75,7 @@ class TestAppleSimulatorOSType(gdbremote
 self.assertIsNotNone(process_info)
 
 # Check that ostype is correct
-self.assertTrue(process_info['ostype'] in platform_names)
+self.assertEquals(process_info['ostype'], platform)
 
 # Now for dylibs
 dylib_info_raw = context.get("dylib_info_raw")
@@ -90,23 +90,23 @@ class TestAppleSimulatorOSType(gdbremote
 break
 
 self.assertIsNotNone(image_info)
-self.assertTrue(image['min_version_os_name'] in platform_names)
+self.assertEquals(image['min_version_os_name'], platform)
 
 
 @apple_simulator_test('iphone')
 @debugserver_test
 def test_simulator_ostype_ios(self):
 self.check_simulator_ostype(sdk='iphonesimulator',
-platform_names=['iphoneos', 'ios'])
+platform='ios')
 
 @apple_simulator_test('appletv')
 @debugserver_test
 def test_simulator_ostype_tvos(self):
 self.check_simulator_ostype(sdk='appletvsimulator',
-platform_names=['tvos'])
+platform='tvos')
 
 @apple_simulator_test('watch')
 @debugserver_test
 def test_simulator_ostype_watchos(self):
 self.check_simulator_ostype(sdk='watchsimulator',
-platform_names=['watchos'], arch='i386')
+platform='watchos', arch='i386')

Modified: lldb/trunk/tools/debugserver/source/MacOSX/MachProcess.mm
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/debugserver/source/MacOSX/MachProcess.mm?rev=330877&r1=330876&r2=330877&view=diff
==
--- lldb/trunk/tools/debugserver/source/MacOSX/MachProcess.mm (original)
+++ lldb/trunk/tools/debugserver/source/MacOSX/MachProcess.mm Wed Apr 25 
15:12:12 2018
@@ -595,7 +595,7 @@ const char *MachProcess::GetDeploymentIn
 
 switch (cmd) {
 case LC_VERSION_MIN_IPHONEOS:
-  return "iphoneos";
+  return "ios";
 case LC_VERSION_MIN_MACOSX:
   return "macosx";
 case LC_VERSION_MIN_TVOS:
@@ -621,7 +621,7 @@ const char *MachProcess::GetDeploymentIn
 case PLATFORM_MACOS:
   return "macosx";
 case PLATFORM_IOS:
-  return "iphoneos";
+  return "ios";
 case PLATFO

[Lldb-commits] [PATCH] D46088: Refactor GetNextPersistentVariableName into a non-virtual method

[Adrian Prantl via Phabricator via lldb-commits]

aprantl created this revision.
aprantl added a reviewer: jingham.

Refactor GetNextPersistentVariableName into a non-virtual method
that takes a prefix string. This simplifies the implementation and
allows plugins such as the Swift plugin to supply different prefixes
for return and error variables.

  

rdar://problem/39722386


https://reviews.llvm.org/D46088

Files:
  include/lldb/Expression/ExpressionVariable.h
  source/Core/ValueObject.cpp
  source/Expression/ExpressionVariable.cpp
  source/Expression/Materializer.cpp
  source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp
  source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h
  source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
  source/Plugins/ExpressionParser/Go/GoUserExpression.cpp
  source/Plugins/ExpressionParser/Go/GoUserExpression.h
  source/Target/ABI.cpp

Index: source/Target/ABI.cpp
===
--- source/Target/ABI.cpp
+++ source/Target/ABI.cpp
@@ -110,8 +110,10 @@
 if (!persistent_expression_state)
   return ValueObjectSP();
 
-ConstString persistent_variable_name(
-persistent_expression_state->GetNextPersistentVariableName(target));
+auto prefix = persistent_expression_state->GetPersistentVariablePrefix();
+ConstString persistent_variable_name =
+persistent_expression_state->GetNextPersistentVariableName(target,
+   prefix);
 
 lldb::ValueObjectSP const_valobj_sp;
 
Index: source/Plugins/ExpressionParser/Go/GoUserExpression.h
===
--- source/Plugins/ExpressionParser/Go/GoUserExpression.h
+++ source/Plugins/ExpressionParser/Go/GoUserExpression.h
@@ -29,8 +29,7 @@
 public:
   GoPersistentExpressionState();
 
-  ConstString GetNextPersistentVariableName(Target &target) override;
-
+  llvm::StringRef GetPersistentVariablePrefix() const override { return "$go"; }
   void RemovePersistentVariable(lldb::ExpressionVariableSP variable) override;
 
   lldb::addr_t LookupSymbol(const ConstString &name) override {
Index: source/Plugins/ExpressionParser/Go/GoUserExpression.cpp
===
--- source/Plugins/ExpressionParser/Go/GoUserExpression.cpp
+++ source/Plugins/ExpressionParser/Go/GoUserExpression.cpp
@@ -272,7 +272,8 @@
   PersistentExpressionState *pv =
   target->GetPersistentExpressionStateForLanguage(eLanguageTypeGo);
   if (pv != nullptr) {
-result->SetName(pv->GetNextPersistentVariableName(*target));
+result->SetName(pv->GetNextPersistentVariableName(
+*target, pv->GetPersistentVariablePrefix()));
 pv->AddVariable(result);
   }
   return lldb::eExpressionCompleted;
@@ -651,16 +652,6 @@
 GoPersistentExpressionState::GoPersistentExpressionState()
 : PersistentExpressionState(eKindGo) {}
 
-ConstString
-GoPersistentExpressionState::GetNextPersistentVariableName(Target &target) {
-  char name_cstr[256];
-  // We can't use the same variable format as clang.
-  ::snprintf(name_cstr, sizeof(name_cstr), "$go%u",
- target.GetNextPersistentVariableIndex());
-  ConstString name(name_cstr);
-  return name;
-}
-
 void GoPersistentExpressionState::RemovePersistentVariable(
 lldb::ExpressionVariableSP variable) {
   RemoveVariable(variable);
Index: source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
===
--- source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
+++ source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
@@ -669,7 +669,7 @@
 }
 
 ConstString ClangUserExpression::ResultDelegate::GetName() {
-  return m_persistent_state->GetNextPersistentVariableName(*m_target_sp);
+  return m_persistent_state->GetNextPersistentVariableName(*m_target_sp, "$");
 }
 
 void ClangUserExpression::ResultDelegate::DidDematerialize(
Index: source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h
===
--- source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h
+++ source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h
@@ -54,16 +54,8 @@
   const CompilerType &compiler_type, lldb::ByteOrder byte_order,
   uint32_t addr_byte_size) override;
 
-  //--
-  /// Return the next entry in the sequence of strings "$0", "$1", ... for
-  /// use naming persistent expression convenience variables.
-  ///
-  /// @return
-  /// A string that contains the next persistent variable name.
-  //--
-  ConstString GetNextPersistentVariableName(Target &target) override;
-
   void RemovePersistentVariable(lldb::ExpressionVariableSP variable) override;
+  llvm::StringRef GetPersistentVariablePrefix() const override