Re: [Lldb-commits] [lldb] b4a6e63 - [lldb/Target] Rework the way the inferior environment is created

[Frédéric Riss via lldb-commits]

Here’s the bot log: 
http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/15055/steps/test/logs/stdio

> On Mar 24, 2020, at 5:10 PM, Adrian McCarthy  wrote:
> 
> Oh, and in case I wasn't clear:  I re-enabled the test TestSettings.py 
> locally.
> 
> On Tue, Mar 24, 2020 at 5:04 PM Adrian McCarthy  <mailto:amcca...@google.com>> wrote:
> I took a stab at this, but I'm not seeing any new test failures.  Can you 
> point me to the specific test and provide a log showing the failures?
> 
> I've been using `ninja check-lldb`, which runs (almost everything) and none 
> of the failures I'm seeing are related to inherit-env.  I assume the problem 
> you're seeing is in TestSettings.py, but I've never figured out how to run 
> individual Python-based lldb tests since all the dotest.py stuff was 
> re-written.  I've started up lldb interactively and tried to emulate the 
> commands that test executes via SBAPI, but everything worked as expected.
> 
> Adrian.
> 
> On Tue, Mar 24, 2020 at 8:19 AM Adrian McCarthy  <mailto:amcca...@google.com>> wrote:
> I'll take a look this morning.
> 
> On Tue, Mar 24, 2020 at 7:00 AM Pavel Labath  <mailto:pa...@labath.sk>> wrote:
> On 23/03/2020 17:17, Frédéric Riss via lldb-commits wrote:
> > The new testing for “inherit-env=false” is failing on Windows. I skipped 
> > the test for now.
> > 
> > Could it be that it never worked there? (In which case XFail would be a 
> > better resolution)
> > Does anyone have easy access to a Windows build to try it out?
> > 
> > Thanks,
> > Fred
> 
> My guess is that this "defensive check"
> <https://github.com/llvm/llvm-project/blob/master/lldb/source/Host/windows/ProcessLauncherWindows.cpp#L26
>  
> <https://github.com/llvm/llvm-project/blob/master/lldb/source/Host/windows/ProcessLauncherWindows.cpp#L26>>
> prevents us from passing a completely blank environment. I am tempted to
> just delete it and see what happens, but maybe Adrian is able to do a
> quick test of this?
> 
> pl

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

Re: [Lldb-commits] [lldb] b4a6e63 - [lldb/Target] Rework the way the inferior environment is created

[Frédéric Riss via lldb-commits]

The new testing for “inherit-env=false” is failing on Windows. I skipped the 
test for now.

Could it be that it never worked there? (In which case XFail would be a better 
resolution)
Does anyone have easy access to a Windows build to try it out?

Thanks,
Fred

> On Mar 23, 2020, at 7:59 AM, Fred Riss via lldb-commits 
>  wrote:
> 
> 
> Author: Fred Riss
> Date: 2020-03-23T07:58:34-07:00
> New Revision: b4a6e63ea12309bf667d1569a20ec5b081cbf2a4
> 
> URL: 
> https://github.com/llvm/llvm-project/commit/b4a6e63ea12309bf667d1569a20ec5b081cbf2a4
> DIFF: 
> https://github.com/llvm/llvm-project/commit/b4a6e63ea12309bf667d1569a20ec5b081cbf2a4.diff
> 
> LOG: [lldb/Target] Rework the way the inferior environment is created
> 
> Summary:
> The interactions between the environment settings (`target.env-vars`,
> `target.inherit-env`) and the inferior life-cycle are non-obvious
> today. For example, if `target.inherit-env` is set, the `target.env-vars`
> setting will be augmented with the contents of the host environment
> the first time the launch environment is queried (usually at
> launch). After that point, toggling `target.inherit-env` will have no
> effect as there's no tracking of what comes from the host and what is
> a user setting.
> 
> This patch computes the environment every time it is queried rather
> than updating the contents of the `target.env-vars` property. This
> means that toggling the `target.inherit-env` property later will now
> have the intended effect.
> 
> This patch also adds a `target.unset-env-vars` settings that one can
> use to remove variables from the launch environment. Using this, you
> can inherit all but a few of the host environment.
> 
> The way the launch environment is constructed is:
>  1/ if `target.inherit-env` is set, then read the host environment
>  into the launch environment.
>  2/ Remove for the environment the variables listed in
>  `target.unset-env`.
>  3/ Augment the launch environment with the contents of
>  `target.env-vars`. This overrides any common values with the host
>  environment.
> 
> The one functional difference here that could be seen as a regression
> is that `target.env-vars` will not contain the inferior environment
> after launch. The patch implements a better alternative in the
> `target show-launch-environment` command which will return the
> environment computed through the above rules.
> 
> Reviewers: labath, jingham
> 
> Subscribers: lldb-commits
> 
> Tags: #lldb
> 
> Differential Revision: https://reviews.llvm.org/D76470
> 
> Added: 
> 
> 
> Modified: 
>lldb/include/lldb/Target/Target.h
>lldb/source/Commands/CommandObjectTarget.cpp
>lldb/source/Target/Target.cpp
>lldb/source/Target/TargetProperties.td
>lldb/test/API/commands/settings/TestSettings.py
> 
> Removed: 
> 
> 
> 
> 
> diff  --git a/lldb/include/lldb/Target/Target.h 
> b/lldb/include/lldb/Target/Target.h
> index 2e7932f49e6f..77cda4998192 100644
> --- a/lldb/include/lldb/Target/Target.h
> +++ b/lldb/include/lldb/Target/Target.h
> @@ -225,9 +225,12 @@ class TargetProperties : public Properties {
>   void DisableASLRValueChangedCallback();
>   void DisableSTDIOValueChangedCallback();
> 
> +  Environment ComputeEnvironment() const;
> +
>   // Member variables.
>   ProcessLaunchInfo m_launch_info;
>   std::unique_ptr m_experimental_properties_up;
> +  Target *m_target;
> };
> 
> class EvaluateExpressionOptions {
> 
> diff  --git a/lldb/source/Commands/CommandObjectTarget.cpp 
> b/lldb/source/Commands/CommandObjectTarget.cpp
> index c70117c7a80a..95f81fc6cd54 100644
> --- a/lldb/source/Commands/CommandObjectTarget.cpp
> +++ b/lldb/source/Commands/CommandObjectTarget.cpp
> @@ -682,6 +682,41 @@ class CommandObjectTargetDelete : public 
> CommandObjectParsed {
>   OptionGroupBoolean m_cleanup_option;
> };
> 
> +class CommandObjectTargetShowLaunchEnvironment : public CommandObjectParsed {
> +public:
> +  CommandObjectTargetShowLaunchEnvironment(CommandInterpreter )
> +  : CommandObjectParsed(
> +interpreter, "target show-launch-environment",
> +"Shows the environment being passed to the process when 
> launched, "
> +"taking info account 3 settings: target.env-vars, "
> +"target.inherit-env and target.unset-env-vars.",
> +nullptr, eCommandRequiresTarget) {}
> +
> +  ~CommandObjectTargetShowLaunchEnvironment() override = default;
> +
> +protected:
> +  bool DoExecute(Args , CommandReturnObject ) override {
> +Target *target = m_exe_ctx.GetTargetPtr();
> +Environment env = target->GetEnvironment();
> +
> +std::vector env_vector;
> +env_vector.reserve(env.size());
> +for (auto  : env)
> +  env_vector.push_back();
> +std::sort(env_vector.begin(), env_vector.end(),
> +  [](Environment::value_type *a, Environment::value_type *b) {
> +return a->first() < b->first();
> +  

Re: [Lldb-commits] [lldb] 6990eaf - [lldb/Test] Skip VSCode test on embedded Darwin

[Frédéric Riss via lldb-commits]

> On Feb 19, 2020, at 5:44 PM, Davide Italiano via lldb-commits 
>  wrote:
> 
> 
> 
>> On Feb 19, 2020, at 17:34, Jonas Devlieghere via lldb-commits 
>>  wrote:
>> 
>> 
>> Author: Jonas Devlieghere
>> Date: 2020-02-19T17:34:01-08:00
>> New Revision: 6990eaf1fe00e9736fbfbcae160e18c5edbcd1d4
>> 
>> URL: 
>> https://github.com/llvm/llvm-project/commit/6990eaf1fe00e9736fbfbcae160e18c5edbcd1d4
>> DIFF: 
>> https://github.com/llvm/llvm-project/commit/6990eaf1fe00e9736fbfbcae160e18c5edbcd1d4.diff
>> 
>> LOG: [lldb/Test] Skip VSCode test on embedded Darwin
>> 
>> These tests are not configured to run on the device.
> 
> 
> I’ve never seen these failing. Did you?

Yes, I did. All of them fail for me and 2 of them hang.

`@skipIfDarwinEmbedded` obviously works for us, but I’m wondering if 
`@skipIfRemote` would not be a better/more generic fix?

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

Re: [Lldb-commits] [lldb] b0937be - Skip TestGuiBasic.py on ios etc device testing.

[Frédéric Riss via lldb-commits]

I think it would be better to stick the equivalent of skipIfRemote in the 
Expect base class.

> On Dec 10, 2019, at 3:15 PM, Jason Molenda via lldb-commits 
>  wrote:
> 
> 
> Author: Jason Molenda
> Date: 2019-12-10T15:15:25-08:00
> New Revision: b0937be06e44c0cdc1c1aac16b76746150e70154
> 
> URL: 
> https://github.com/llvm/llvm-project/commit/b0937be06e44c0cdc1c1aac16b76746150e70154
> DIFF: 
> https://github.com/llvm/llvm-project/commit/b0937be06e44c0cdc1c1aac16b76746150e70154.diff
> 
> LOG: Skip TestGuiBasic.py on ios etc device testing.
> 
> Added: 
> 
> 
> Modified: 
>lldb/packages/Python/lldbsuite/test/commands/gui/basic/TestGuiBasic.py
> 
> Removed: 
> 
> 
> 
> 
> diff  --git 
> a/lldb/packages/Python/lldbsuite/test/commands/gui/basic/TestGuiBasic.py 
> b/lldb/packages/Python/lldbsuite/test/commands/gui/basic/TestGuiBasic.py
> index d501b266cc12..0ee0982a5b42 100644
> --- a/lldb/packages/Python/lldbsuite/test/commands/gui/basic/TestGuiBasic.py
> +++ b/lldb/packages/Python/lldbsuite/test/commands/gui/basic/TestGuiBasic.py
> @@ -15,6 +15,7 @@ class BasicGuiCommandTest(PExpectTest):
> # under ASAN on a loaded machine..
> @skipIfAsan
> @skipIfCursesSupportMissing
> +@skipIfDarwinEmbedded # "run" command will not work correctly for remote 
> debug
> def test_gui(self):
> self.build()
> 
> 
> 
> 
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

Re: [Lldb-commits] [lldb] r374335 - [lldb] Make sure import-std-module/sysroot actually passes for the right reasons

[Frédéric Riss via lldb-commits]

> On Oct 10, 2019, at 5:57 AM, Raphael Isemann via lldb-commits 
>  wrote:
> 
> Author: teemperor
> Date: Thu Oct 10 05:57:14 2019
> New Revision: 374335
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=374335=rev
> Log:
> [lldb] Make sure import-std-module/sysroot actually passes for the right 
> reasons
> 
> This test was previously passing because myabs was actually emitted into the
> debug information and we called that. The test itself was broken as it didn't
> use the libc++ directory structure (the /v1/ directory was just called 
> /include/).
> 
> This patch gives myabs a default argument which we can't get from debug 
> information

Just FYI: debug information can express default arguments even though Clang 
doesn’t emit this for now. At some point in the future, this test could stop 
testing what you expect. Would it work to force the function to not have any 
debug info (I think __attribute__(no_debug_info)) would do that.

> and inlines the function to make sure we can't call it from LLDB without 
> loading
> the C++ module.
> 
> Added:
>
> lldb/trunk/packages/Python/lldbsuite/test/commands/expression/import-std-module/sysroot/root/usr/include/c++/v1/
>
> lldb/trunk/packages/Python/lldbsuite/test/commands/expression/import-std-module/sysroot/root/usr/include/c++/v1/algorithm
>
> lldb/trunk/packages/Python/lldbsuite/test/commands/expression/import-std-module/sysroot/root/usr/include/c++/v1/module.modulemap
> Removed:
>
> lldb/trunk/packages/Python/lldbsuite/test/commands/expression/import-std-module/sysroot/root/usr/include/c++/include/
> Modified:
>
> lldb/trunk/packages/Python/lldbsuite/test/commands/expression/import-std-module/sysroot/Makefile
>
> lldb/trunk/packages/Python/lldbsuite/test/commands/expression/import-std-module/sysroot/TestStdModuleSysroot.py
>
> lldb/trunk/packages/Python/lldbsuite/test/commands/expression/import-std-module/sysroot/main.cpp
> 
> Modified: 
> lldb/trunk/packages/Python/lldbsuite/test/commands/expression/import-std-module/sysroot/Makefile
> URL: 
> http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/commands/expression/import-std-module/sysroot/Makefile?rev=374335=374334=374335=diff
> ==
> --- 
> lldb/trunk/packages/Python/lldbsuite/test/commands/expression/import-std-module/sysroot/Makefile
>  (original)
> +++ 
> lldb/trunk/packages/Python/lldbsuite/test/commands/expression/import-std-module/sysroot/Makefile
>  Thu Oct 10 05:57:14 2019
> @@ -3,7 +3,7 @@
> # system headers.
> NO_TEST_COMMON_H := 1
> 
> -CXXFLAGS_EXTRAS = -I $(SRCDIR)/root/usr/include/c++/include/ -I 
> $(SRCDIR)/root/usr/include/ -nostdinc -nostdinc++
> +CXXFLAGS_EXTRAS = -I $(SRCDIR)/root/usr/include/c++/v1/ -I 
> $(SRCDIR)/root/usr/include/ -nostdinc -nostdinc++
> CXX_SOURCES := main.cpp
> 
> include Makefile.rules
> 
> Modified: 
> lldb/trunk/packages/Python/lldbsuite/test/commands/expression/import-std-module/sysroot/TestStdModuleSysroot.py
> URL: 
> http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/commands/expression/import-std-module/sysroot/TestStdModuleSysroot.py?rev=374335=374334=374335=diff
> ==
> --- 
> lldb/trunk/packages/Python/lldbsuite/test/commands/expression/import-std-module/sysroot/TestStdModuleSysroot.py
>  (original)
> +++ 
> lldb/trunk/packages/Python/lldbsuite/test/commands/expression/import-std-module/sysroot/TestStdModuleSysroot.py
>  Thu Oct 10 05:57:14 2019
> @@ -27,4 +27,6 @@ class ImportStdModule(TestBase):
> 
> # Call our custom function in our sysroot std module.
> # If this gives us the correct result, then we used the sysroot.
> -self.expect("expr std::myabs(-42)", substrs=['(int) $0 = 42'])
> +# We rely on the default argument of -123 to make sure we actually 
> have the C++ module.
> +# (We don't have default arguments in the debug information).
> +self.expect("expr std::myabs()", substrs=['(int) $0 = 123'])
> 
> Modified: 
> lldb/trunk/packages/Python/lldbsuite/test/commands/expression/import-std-module/sysroot/main.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/commands/expression/import-std-module/sysroot/main.cpp?rev=374335=374334=374335=diff
> ==
> --- 
> lldb/trunk/packages/Python/lldbsuite/test/commands/expression/import-std-module/sysroot/main.cpp
>  (original)
> +++ 
> lldb/trunk/packages/Python/lldbsuite/test/commands/expression/import-std-module/sysroot/main.cpp
>  Thu Oct 10 05:57:14 2019
> @@ -2,5 +2,6 @@
> 
> int main(int argc, char **argv) {
>   libc_struct s;
> +  std::vector v;
>   return 0; // Set break point at this line.
> }
> 
> Added: 
> 

Re: [Lldb-commits] [PATCH] D67520: Add pretty printing of Clang "bitfield" enums

[Frédéric Riss via lldb-commits]

So I should see this on my machine, but I don’t...

> On Oct 8, 2019, at 10:06 AM, Shafik Yaghmour via Phabricator 
>  wrote:
> 
> shafik added a comment.
> 
> See the same break on green dragon as well: 
> http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/2360/testReport/LLDB/SymbolFile_NativePDB/s_constant_cpp/
> 
> 
> Repository:
>  rG LLVM Github Monorepo
> 
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D67520/new/
> 
> https://reviews.llvm.org/D67520
> 
> 
> 

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

Re: [Lldb-commits] [PATCH] D67520: Add pretty printing of Clang "bitfield" enums

[Frédéric Riss via lldb-commits]

Taking a look

> On Oct 8, 2019, at 9:47 AM, Stella Stamenova via Phabricator 
>  wrote:
> 
> stella.stamenova added a comment.
> 
> It looks like this broke one of the tests on the Windows LLDB bot:
> 
> http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/9628/steps/test/logs/stdio
> 
> The bot was already red because of a change from yesterday, so you probably 
> didn't get a notification.
> 
> 
> Repository:
>  rG LLVM Github Monorepo
> 
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D67520/new/
> 
> https://reviews.llvm.org/D67520
> 
> 
> 

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

Re: [Lldb-commits] [lldb] r370848 - [lldb][NFC] Add a simple test for thread_local storage.

[Frédéric Riss via lldb-commits]

> On Sep 4, 2019, at 1:02 AM, Raphael Isemann via lldb-commits 
>  wrote:
> 
> Author: teemperor
> Date: Wed Sep  4 01:02:52 2019
> New Revision: 370848
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=370848=rev
> Log:
> [lldb][NFC] Add a simple test for thread_local storage.
> 
> Seems we fail to read TLS data on Linux, so the test only runs on
> macOS for now. We will see how this test runs on the BSD bots.
> 
> Added:
>lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/thread_local/
>lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/thread_local/Makefile
>
> lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/thread_local/TestThreadLocal.py
>lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/thread_local/main.cpp
> 
> Added: 
> lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/thread_local/Makefile
> URL: 
> http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/thread_local/Makefile?rev=370848=auto
> ==
> --- lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/thread_local/Makefile 
> (added)
> +++ lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/thread_local/Makefile 
> Wed Sep  4 01:02:52 2019
> @@ -0,0 +1,3 @@
> +LEVEL = ../../../make
> +CXX_SOURCES := main.cpp
> +include $(LEVEL)/Makefile.rules
> 
> Added: 
> lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/thread_local/TestThreadLocal.py
> URL: 
> http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/thread_local/TestThreadLocal.py?rev=370848=auto
> ==
> --- 
> lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/thread_local/TestThreadLocal.py
>  (added)
> +++ 
> lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/thread_local/TestThreadLocal.py
>  Wed Sep  4 01:02:52 2019
> @@ -0,0 +1,5 @@
> +from lldbsuite.test import lldbinline
> +from lldbsuite.test import decorators
> +
> +lldbinline.MakeInlineTest(__file__, globals(),
> +  lldbinline.expectedFailureAll(oslist=["windows", 
> "linux"]))
> 
> Added: 
> lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/thread_local/main.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/thread_local/main.cpp?rev=370848=auto
> ==
> --- lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/thread_local/main.cpp 
> (added)
> +++ lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/thread_local/main.cpp 
> Wed Sep  4 01:02:52 2019
> @@ -0,0 +1,17 @@
> +int storage = 45;
> +thread_local int tl_global_int = 123;
> +thread_local int *tl_global_ptr = 
> +
> +int main(int argc, char **argv) {
> +  //% self.expect("expr tl_local_int", error=True, substrs=["couldn't get 
> the value of variable tl_local_int"])
> +  //% self.expect("expr *tl_local_ptr", error=True, substrs=["couldn't get 
> the value of variable tl_local_ptr"])
> +  thread_local int tl_local_int = 321;
> +  thread_local int *tl_local_ptr = nullptr;
> +  tl_local_ptr = _local_int;
> +  tl_local_int++;
> +  //% self.expect("expr tl_local_int + 1", substrs=["int", "= 323"])
> +  //% self.expect("expr *tl_local_ptr + 2", substrs=["int", "= 324"])
> +  //% self.expect("expr tl_global_int", substrs=["int", "= 123"])
> +  //% self.expect("expr *tl_global_ptr", substrs=["int", "= 45"])
> +  return 0;
> +}

Is frame variable able to get the value of the TLS variables, or do we rely on 
the Clang codeine here? I don’t remember if accessing TLS requires a function 
call on Darwin.

Fred

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

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

Re: [Lldb-commits] [PATCH] D61044: Fix infinite recursion when calling C++ template functions

[Frédéric Riss via lldb-commits]

> On Apr 23, 2019, at 3:48 PM, Frederic Riss via Phabricator 
>  wrote:
> 
> friss created this revision.
> friss added reviewers: shafik, clayborg, jingham.
> Herald added subscribers: kristof.beyls, javed.absar, aprantl.
> 
> When we encounter a templated function in the debug information, we
> were creating an AST that looked like this:
> 
> FunctionTemplateDecl 0x12980ab90 <>  foo
> 
> | -TemplateTypeParmDecl 0x12980aad0 <>  class 
> depth 0 index 0 T |
> | -FunctionDecl 0x12980aa30 <>  foo 'int 
> (int)' extern |
> | 
>   | -TemplateArgument type 'int' |
> | `-ParmVarDecl 0x12980a998 <>  t1 'int'  
>   |
> |
> 
> `-FunctionDecl 0x12980aa30 <>  foo 'int 
> (int)' extern
> 
>  |-TemplateArgument type 'int'
>  `-ParmVarDecl 0x12980a998 <>  t1 'int'
> 
> Note that the FunctionTemplateDecl has 2 children which are identical (as
> in have the same address). This is not what Clang is doing:
> 
> FunctionTemplateDecl 0x7f89d206c6f8  
> line:2:5 foo
> 
> | -TemplateTypeParmDecl 0x7f89d206c4a8  col:19 referenced 
> typename depth 0 index 0 T |
> | -FunctionDecl 0x7f89d206c660  line:2:5 foo 'int (T)'
>   |
> | `-ParmVarDecl 0x7f89d206c570  col:11 t1 'T'  
>   |
> |
> 
> `-FunctionDecl 0x7f89d206cb60  line:2:5 used foo 'int 
> (int)'
> 
>  |-TemplateArgument type 'int'
>  `-ParmVarDecl 0x7f89d206ca68  col:11 t1 'int':’int'

I don’t know what Phabricator did to my commit message. Here it is (hopefully) 
un-mangled:

Fix infinite recursion when calling C++ template functions

Summary:
When we encounter a templated function in the debug information, we
were creating an AST that looked like this:

FunctionTemplateDecl 0x12980ab90 <>  foo
|-TemplateTypeParmDecl 0x12980aad0 <>  class 
depth 0 index 0 T
|-FunctionDecl 0x12980aa30 <>  foo 'int 
(int)' extern
| |-TemplateArgument type 'int'
| `-ParmVarDecl 0x12980a998 <>  t1 'int'
`-FunctionDecl 0x12980aa30 <>  foo 'int 
(int)' extern
  |-TemplateArgument type 'int'
  `-ParmVarDecl 0x12980a998 <>  t1 'int'

Note that the FunctionTemplateDecl has 2 children which are identical (as
in have the same address). This is not what Clang is doing:

FunctionTemplateDecl 0x7f89d206c6f8  
line:2:5 foo
|-TemplateTypeParmDecl 0x7f89d206c4a8  col:19 referenced 
typename depth 0 index 0 T
|-FunctionDecl 0x7f89d206c660  line:2:5 foo 'int (T)'
| `-ParmVarDecl 0x7f89d206c570  col:11 t1 'T'
`-FunctionDecl 0x7f89d206cb60  line:2:5 used foo 'int 
(int)'
  |-TemplateArgument type 'int'
  `-ParmVarDecl 0x7f89d206ca68  col:11 t1 'int':'int'

The 2 chidlren are different and actually repesent different things: the 
first
one is the unspecialized version and the second one is specialized. (Just 
looking
at the names shows another major difference which is that we create the 
parent
with a name of "foo" when it should be just "foo".)

The fact that we have those 2 identical children confuses the ClangImporter
and generates an infinite recursion (reported in https://llvm.org/pr41473).
We cannot create the unspecialized version as the debug information doesn't
contain a mapping from the template parameters to their use in the 
prototype.

This patch just creates 2 different FunctionDecls for those 2 children of 
the
FunctionTemplateDecl. This avoids the infinite recursion and allows us to
call functions. As the XFAILs in the added test show, we've still got issues
in our handling of templates. I believe they are mostly centered on the fact
that we create do not register "foo" as a template, but "foo". This is
a bigger change that will need changes to the debug information generation.
I believe this change makes sense on its own.

Reviewers: shafik, clayborg, jingham

Subscribers: aprantl, javed.absar, kristof.beyls, lldb-commits

Differential Revision: https://reviews.llvm.org/D61044

Fred

> The 2 chidlren are different and actually repesent different things: the first
> one is the unspecialized version and the second one is specialized. (Just 
> looking
> at the names shows another major difference which is that we create the parent
> with a name of "foo" when it should be just "foo".)
> 
> The fact that we have those 2 identical children confuses the ClangImporter
> and generates an infinite recursion (reported in https://llvm.org/pr41473).
> We cannot create the unspecialized version as the debug information doesn't
> contain a mapping from the template parameters to their use in the prototype.
> 
> This patch just creates 2 different FunctionDecls for those 2 children of the
> FunctionTemplateDecl. This avoids the infinite recursion and allows us to
> call functions. As the XFAILs in the added test show, we've still got 

Re: [Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

[Frédéric Riss via lldb-commits]

> On Oct 15, 2018, at 4:40 PM, Vedant Kumar  wrote:
> 
> 
> 
>> On Oct 15, 2018, at 3:47 PM, Stella Stamenova via Phabricator 
>>  wrote:
>> 
>> stella.stamenova added a comment.
>> 
>> In https://reviews.llvm.org/D50478#1262717, @vsk wrote:
>> 
>>> In https://reviews.llvm.org/D50478#1262710, @stella.stamenova wrote:
>>> 
 Unfortunately, the bots are broken because of the FileCheck issue, so I 
 can't confirm with them, but I see a number of these tests fail in our 
 local testing. Some fail on both Windows and Linux and some just fail on 
 Linux. Here are the failing tests:
 
 Linux:
 lldb-Suite :: 
 functionalities/tail_call_frames/disambiguate_call_site/TestDisambiguateCallSite.py
 lldb-Suite :: 
 functionalities/tail_call_frames/disambiguate_paths_to_common_sink/TestDisambiguatePathsToCommonSink.py
 lldb-Suite :: 
 functionalities/tail_call_frames/disambiguate_tail_call_seq/TestDisambiguateTailCallSeq.py
 lldb-Suite :: 
 functionalities/tail_call_frames/inlining_and_tail_calls/TestInliningAndTailCalls.py
 lldb-Suite :: 
 functionalities/tail_call_frames/sbapi_support/TestTailCallFrameSBAPI.py
 lldb-Suite :: 
 functionalities/tail_call_frames/thread_step_out_message/TestArtificialFrameStepOutMessage.py
 lldb-Suite :: 
 functionalities/tail_call_frames/unambiguous_sequence/TestUnambiguousTailCalls.py
 
 Windows:
 lldb-Suite :: 
 functionalities/tail_call_frames/sbapi_support/TestTailCallFrameSBAPI.py
 lldb-Suite :: 
 functionalities/tail_call_frames/thread_step_out_or_return/TestSteppingOutWithArtificialFrames.py
 
 
 Let me know what you need to investigate.
>>> 
>>> 
>>> Strange, I didn't get any bot failure notifications in the days after this 
>>> landed. Could you share the output from the failing tests?
>> 
>> 
>> All the failures on Windows are happening when validating the function name. 
>> For example:
>> 
>> ==
>> 
>> FAIL: test_tail_call_frame_sbapi 
>> (TestTailCallFrameSBAPI.TestTailCallFrameSBAPI)
>> 
>> --
>> 
>> Traceback (most recent call last):
>> 
>>   File 
>> "E:\_work\55\s\llvm\tools\lldb\packages\Python\lldbsuite\test\functionalities\tail_call_frames\sbapi_support\TestTailCallFrameSBAPI.py",
>>  line 19, in test_tail_call_frame_sbapi
>> 
>> self.do_test()
>> 
>>   File 
>> "E:\_work\55\s\llvm\tools\lldb\packages\Python\lldbsuite\test\functionalities\tail_call_frames\sbapi_support\TestTailCallFrameSBAPI.py",
>>  line 64, in do_test
>> 
>> self.assertTrue(frame.GetDisplayFunctionName() == name)
> 
> It could be that the display name of a function is formatted differently on 
> Windows. Do you have an easy way of determining what 
> frame.GetDisplayFunctionName() is?

If you use assertEqual(a,b) instead of assertTrue, it will print out the values 
and make it easier to debug.

Fred 

> 
>> 
>> AssertionError: False is not True
>> 
>> Config=x86_64-E:\_work\55\b\LLVMBuild\Release\bin\clang.exe
>> 
>> --
>> 
>> There are several different failures on Linux. Here's the first one:
>> 
>> FAIL: LLDB (/vstsdrive/_work/38/b/LLVMBuild/bin/clang-8-x86_64) :: 
>> test_dwarf (lldbsuite.test.lldbtest.TestDisambiguateCallSite)
>> 
>> --- FileCheck trace (code=1) ---
>> /vstsdrive/_work/38/b/LLVMBuild/bin/FileCheck 
>> /vstsdrive/_work/38/s/llvm/tools/lldb/packages/Python/lldbsuite/test/functionalities/tail_call_frames/disambiguate_call_site/main.cpp
>>  -implicit-check-not=artificial
>> 
>> FileCheck input:
>> * thread #1, name = 'a.out', stop reason = breakpoint 1.1
>>   * frame #0: 0x00400580 a.out`sink() at main.cpp:13:4 [opt]
>> frame #1: 0x004005b8 a.out`main(argc=1, (null)=) at 
>> main.cpp:28:3 [opt]
>> frame #2: 0x7f980aff7830 libc.so.6`__libc_start_main + 240
>> frame #3: 0x004004a9 a.out`_start + 41
> 
> It looks like we're not generating tail call frames on Linux at all. It would 
> help to have logs from "log enable -f /tmp/linux-stepping.log lldb step".
> 
> I'm headed out of the office now, but If you need to disable the tests on 
> Windows/Linux , the fastest way to do that would be to add a platform check 
> to skipUnlessHasCallSiteInfo in decorators.py.
> 
> vedant
> 
> 
>> 
>> 
>> FileCheck output:
>> 
>> /vstsdrive/_work/38/s/llvm/tools/lldb/packages/Python/lldbsuite/test/functionalities/tail_call_frames/disambiguate_call_site/main.cpp:15:17:
>>  error: CHECK-NEXT: expected string not found in input
>>  // CHECK-NEXT: func2{{.*}} [opt] [artificial]
>> ^
>> :3:2: note: scanning from here
>>  frame #1: 0x004005b8 a.out`main(argc=1, (null)=) at 
>> main.cpp:28:3 [opt]
>>  ^
>> :3:80: note: possible intended match here
>>  frame #1: 0x004005b8 a.out`main(argc=1, (null)=) at 
>> 

Re: [Lldb-commits] [PATCH] D48393: Make DWARFParsing more thread-safe

[Frédéric Riss via lldb-commits]

> On Jun 21, 2018, at 8:03 AM, Pavel Labath via Phabricator 
>  wrote:
> 
> labath added a comment.
> 
> In https://reviews.llvm.org/D48393#1139270, @clayborg wrote:
> 
>> In https://reviews.llvm.org/D48393#1138989, @labath wrote:
>> 
>>> I am not sure this will actually solve the problems you are seeing. This 
>>> may avoid corrupting the internal DenseMap data structures, but it will not 
>>> make the algorithm using them actually correct.
>>> For example the pattern in `ParseTypeFromDWARF` is:
>>> 
>>> 1. check the "already parsed map". If the DIE is already parsed then you're 
>>> done.
>>> 2. if the map contains the magic "DIE_IS_BEING_PARSED" key, abort 
>>> (recursive dwarf references)
>>> 3. otherwise, insert the  "DIE_IS_BEING_PARSED" key into the map
>>> 4. do the parsing, which potentially involves recursive 
>>> `ParseTypeFromDWARF` calls
>>> 5. insert the parsed type into the map
>>> 
>>>  What you do is make each of the steps (1), (3), (5) atomic individually. 
>>> However, the whole algorithm is not correct unless the whole sequence is 
>>> atomic as a whole. Otherwise, if you have two threads trying to parse the 
>>> same DIE (directly or indirectly), one of them could see the intermediate 
>>> DIE_IS_BEING_PARSED and incorrectly assume that it encountered recursive 
>>> types.
>> 
>> 
>> We need to make #1 atomic.
>> #2 would need to somehow know if the type is already being parsed 
>> recursively by the current thread. If so, then do what we do now. If not, we 
>> need a way to wait on the completion of this type so the other parsing 
>> thread can complete it and put it into the map, at which time we grab the 
>> right value from the map
>> So #6 step would need to be added so after we do put it into the map, we can 
>> notify other threads that might be waiting
> 
> 
> It's even more complicated than that, in case you really have reference 
> cycles, you can have multiple threads starting parsing from different points 
> in that cycle, and getting deadlocked waiting for the DIE_IS_BEING_PARSED 
> results from each other.
> 
> The only sane algorithm I can come up right now is to make the list of parsed 
> dies local to each thread/parsing entity (e.g. via a "visited" list), and 
> only update the global map once the parsing has completed (successfully or 
> not). This can potentially duplicate some effort where one thread parses a 
> type only to find out that it has already been parsed, but hopefully that is 
> not going to be the common case. The alternative is some complicated resource 
> cycle detection scheme.

Doh, that sounds like what I just tried to describe in my other mail. I should 
read threads completely before replying.

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

Re: [Lldb-commits] [PATCH] D48393: Make DWARFParsing more thread-safe

[Frédéric Riss via lldb-commits]

> On Jun 21, 2018, at 3:18 AM, Pavel Labath via Phabricator 
>  wrote:
> 
> labath added a comment.
> 
> I am not sure this will actually solve the problems you are seeing. This may 
> avoid corrupting the internal DenseMap data structures, but it will not make 
> the algorithm using them actually correct.
> For example the pattern in `ParseTypeFromDWARF` is:
> 
> 1. check the "already parsed map". If the DIE is already parsed then you're 
> done.
> 2. if the map contains the magic "DIE_IS_BEING_PARSED" key, abort (recursive 
> dwarf references)
> 3. otherwise, insert the  "DIE_IS_BEING_PARSED" key into the map
> 4. do the parsing, which potentially involves recursive `ParseTypeFromDWARF` 
> calls
> 5. insert the parsed type into the map
> 
> What you do is make each of the steps (1), (3), (5) atomic individually. 
> However, the whole algorithm is not correct unless the whole sequence is 
> atomic as a whole. Otherwise, if you have two threads trying to parse the 
> same DIE (directly or indirectly), one of them could see the intermediate 
> DIE_IS_BEING_PARSED and incorrectly assume that it encountered recursive 
> types.
> 
> So, I think that locking at a higher level would be better. Doing that will 
> certainly be tricky though…

You are absolutely correct. I had quickly thought about this, but thought that 
we would just be duplicating work. Seeing how DIE_IS_BEING_PARSED is used this 
is actually a correctness issue.

While looking at this and especially the DIE_BEING_PARSED stuff, I was 
wondering if we couldn’t use a lockless data-structure like a hand-rolled 
bit-vector instead of using the map to store this information. What if we do 
something like this, but we make the DIE_IS_BEING_PARSED data-structure 
thread-local? In this case, I suppose you would potentially duplicate some 
work, but I think it should yield a correct result. WDYT?

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

Re: [Lldb-commits] [RFC] Type lookup for template types is broken...

[Frédéric Riss via lldb-commits]

> On May 8, 2018, at 9:04 AM, Greg Clayton <clayb...@gmail.com> wrote:
> 
> The only way for us to find all classes whose type is "C" is to add the entry 
> for all template classes named "C", so I would vote to add them as it is 
> accurate. Do we currently add one for "C<12, 16>”?

Yes we do, not sure it is actually useful.

Fred

> 
> Greg
> 
>> On May 8, 2018, at 8:58 AM, Frédéric Riss via lldb-commits 
>> <lldb-commits@lists.llvm.org <mailto:lldb-commits@lists.llvm.org>> wrote:
>> 
>> 
>> 
>>> On May 8, 2018, at 8:30 AM, paul.robin...@sony.com 
>>> <mailto:paul.robin...@sony.com> wrote:
>>> 
>>> 
>>> 
>>>> -Original Message-
>>>> From: lldb-commits [mailto:lldb-commits-boun...@lists.llvm.org 
>>>> <mailto:lldb-commits-boun...@lists.llvm.org>] On Behalf
>>>> Of Pavel Labath via lldb-commits
>>>> Sent: Tuesday, May 08, 2018 10:48 AM
>>>> To: fr...@apple.com <mailto:fr...@apple.com>
>>>> Cc: lldb-commits@lists.llvm.org <mailto:lldb-commits@lists.llvm.org>
>>>> Subject: Re: [Lldb-commits] [RFC] Type lookup for template types is
>>>> broken...
>>>> 
>>>> Well.. it encodes some assumptions about how a class name looks like,
>>>> which
>>>> are probably valid for C++, but they don't have to hold for any language
>>>> frontend LLVM supports. That said, I am not saying this is worth the
>>>> trouble of adding a special "these are the additional names you are to
>>>> insert into the index" channel that clang should use to communicate this
>>>> (I
>>>> wouldn't be surprised if we make even stronger assumptions elsewhere). I
>>>> was just curious about what your thoughts here were.
>>> 
>>> If you add an accelerator entry for "C" what does it point to?  All the
>>> instantiations of "C"?  The DWARF does not describe the template, only
>>> the concrete instances.
>> 
>> Yes, there would be a “C” entry for every instantiation of C.
>> 
>> Fred
>> 
>>> --paulr
>>> 
>>> 
>>>> On Tue, 8 May 2018 at 15:29, Frédéric Riss <fr...@apple.com 
>>>> <mailto:fr...@apple.com>> wrote:
>>>> 
>>>> 
>>>> 
>>>>> On May 8, 2018, at 2:23 AM, Pavel Labath <lab...@google.com 
>>>>> <mailto:lab...@google.com>> wrote:
>>>> 
>>>>> I am still building a picture for myself of how the accelerator tables
>>>> and
>>>>> our name lookup works, but from what I managed to learn so far, adding
>>>> an
>>>>> accelerator for "C" seems like a useful thing to do. However, this does
>>>> go
>>>>> beyond what the DWARF 5 spec says we should do (we are only required to
>>>> add
>>>>> the DW_AT_name string). We are still free to add any extra entries we
>>>> like,
>>>>> but if we're going to be relying on this, we should try to get some of
>>>> this
>>>>> into the next version of the spec.
>>>> 
>>>> 
>>>>> On Mon, 7 May 2018 at 22:19, Frédéric Riss via lldb-commits <
>>>>> lldb-commits@lists.llvm.org <mailto:lldb-commits@lists.llvm.org>> wrote:
>>>> 
>>>>> (...At least when using accelerator tables)
>>>> 
>>>> 
>>>>> If you apply the following patch, TestClassTemplateParameterPack.py will
>>>> 
>>>>> start failing:
>>>> 
>>>>> diff --git
>>>> 
>>>> 
>>>> a/packages/Python/lldbsuite/test/lang/cpp/class-template-parameter-
>>>> pack/main.cpp
>>>> 
>>>> b/packages/Python/lldbsuite/test/lang/cpp/class-template-parameter-
>>>> pack/main.cpp
>>>> 
>>>>> index 90e63b40f..304872a15 100644
>>>>> ---
>>>> 
>>>> 
>>>> a/packages/Python/lldbsuite/test/lang/cpp/class-template-parameter-
>>>> pack/main.cpp
>>>> 
>>>>> +++
>>>> 
>>>> 
>>>> b/packages/Python/lldbsuite/test/lang/cpp/class-template-parameter-
>>>> pack/main.cpp
>>>> 
>>>>> @@ -37,7 +37,7 @@ template <> struct D<int, int, bool> : D<int, int> {
>>>> 
>>>> 
>>>> 
>&g

Re: [Lldb-commits] [RFC] Type lookup for template types is broken...

[Frédéric Riss via lldb-commits]

> On May 8, 2018, at 8:30 AM, paul.robin...@sony.com wrote:
> 
> 
> 
>> -Original Message-
>> From: lldb-commits [mailto:lldb-commits-boun...@lists.llvm.org 
>> <mailto:lldb-commits-boun...@lists.llvm.org>] On Behalf
>> Of Pavel Labath via lldb-commits
>> Sent: Tuesday, May 08, 2018 10:48 AM
>> To: fr...@apple.com <mailto:fr...@apple.com>
>> Cc: lldb-commits@lists.llvm.org <mailto:lldb-commits@lists.llvm.org>
>> Subject: Re: [Lldb-commits] [RFC] Type lookup for template types is
>> broken...
>> 
>> Well.. it encodes some assumptions about how a class name looks like,
>> which
>> are probably valid for C++, but they don't have to hold for any language
>> frontend LLVM supports. That said, I am not saying this is worth the
>> trouble of adding a special "these are the additional names you are to
>> insert into the index" channel that clang should use to communicate this
>> (I
>> wouldn't be surprised if we make even stronger assumptions elsewhere). I
>> was just curious about what your thoughts here were.
> 
> If you add an accelerator entry for "C" what does it point to?  All the
> instantiations of "C"?  The DWARF does not describe the template, only
> the concrete instances.

Yes, there would be a “C” entry for every instantiation of C.

Fred

> --paulr
> 
> 
>> On Tue, 8 May 2018 at 15:29, Frédéric Riss <fr...@apple.com> wrote:
>> 
>> 
>> 
>>> On May 8, 2018, at 2:23 AM, Pavel Labath <lab...@google.com> wrote:
>> 
>>> I am still building a picture for myself of how the accelerator tables
>> and
>>> our name lookup works, but from what I managed to learn so far, adding
>> an
>>> accelerator for "C" seems like a useful thing to do. However, this does
>> go
>>> beyond what the DWARF 5 spec says we should do (we are only required to
>> add
>>> the DW_AT_name string). We are still free to add any extra entries we
>> like,
>>> but if we're going to be relying on this, we should try to get some of
>> this
>>> into the next version of the spec.
>> 
>> 
>>> On Mon, 7 May 2018 at 22:19, Frédéric Riss via lldb-commits <
>>> lldb-commits@lists.llvm.org> wrote:
>> 
>>> (...At least when using accelerator tables)
>> 
>> 
>>> If you apply the following patch, TestClassTemplateParameterPack.py will
>> 
>>> start failing:
>> 
>>> diff --git
>> 
>> 
>> a/packages/Python/lldbsuite/test/lang/cpp/class-template-parameter-
>> pack/main.cpp
>> 
>> b/packages/Python/lldbsuite/test/lang/cpp/class-template-parameter-
>> pack/main.cpp
>> 
>>> index 90e63b40f..304872a15 100644
>>> ---
>> 
>> 
>> a/packages/Python/lldbsuite/test/lang/cpp/class-template-parameter-
>> pack/main.cpp
>> 
>>> +++
>> 
>> 
>> b/packages/Python/lldbsuite/test/lang/cpp/class-template-parameter-
>> pack/main.cpp
>> 
>>> @@ -37,7 +37,7 @@ template <> struct D<int, int, bool> : D<int, int> {
>> 
>> 
>> 
>> 
>>>  int main (int argc, char const *argv[])
>>>  {
>>> -C<int,16,32> myC;
>>> +C<int,16,32> myC; //% self.runCmd("settings set
>> 
>>> target.experimental.inject-local-vars false")
>> 
>>>  C<int,16> myLesserC;
>>>  myC.member = 64;
>>>  (void)C<int,16,32>().isSixteenThirtyTwo();
>> 
>> 
>>> The test does things like invoke methods on temporary template objects:
>>> //% self.expect("expression -- C<int, 16>().isSixteenThirtyTwo()",
>> 
>>> DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ["false"])
>> 
>>> The above expression currently works because there’s a local of type
>> 
>>> C<int, 16>. With injected locals, the type is made readily available to
>>> Clang. No type lookup is required for this to work in this setup.
>> 
>>> If you stop injecting locals, the test fails. We don’t provide the
>> 
>>> information to Clang to understand what C is. The reason is that when
>> Clang
>>> parses “C”, it is going to ask about “C”, not the fully
>> templated
>>> name. Our accelerator tables contain references to the full names, but
>> not
>>> to C alone and we never find it. If I change Clang and dsymutil to add
>> an
>>> accelerator for “C” each time an instance of C is seen then it 

Re: [Lldb-commits] [PATCH] D46551: Inject only relevant local variables in the expression evaluation context

[Frédéric Riss via lldb-commits]

> On May 8, 2018, at 1:32 AM, Pavel Labath via Phabricator 
>  wrote:
> 
> labath added a comment.
> 
> One could probably concoct an example (using macros, token pasting and such) 
> where this would get it wrong, but that's probably not worth supporting.

Yeah, I thought about this and I’m not sure it’s worth supporting. We could 
support it by doing the filtering after a round of pre-processing, but it seems 
really overkill. 

> 
> 
> 
> Comment at: source/Expression/ExpressionSourceCode.cpp:193
> +var_name == ConstString(".block_descriptor") ||
> +!ExprBodyContainsVar(var_name.AsCString(), expr))
>   continue;
> 
> `s/AsCString/GetStringRef` (saves a `strlen` operation).

Thanks,
Fred

> 
> https://reviews.llvm.org/D46551
> 
> 
> 

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

[Lldb-commits] [RFC] Type lookup for template types is broken...

[Frédéric Riss via lldb-commits]

(...At least when using accelerator tables)

If you apply the following patch, TestClassTemplateParameterPack.py will start 
failing:
diff --git 
a/packages/Python/lldbsuite/test/lang/cpp/class-template-parameter-pack/main.cpp
 
b/packages/Python/lldbsuite/test/lang/cpp/class-template-parameter-pack/main.cpp
index 90e63b40f..304872a15 100644
--- 
a/packages/Python/lldbsuite/test/lang/cpp/class-template-parameter-pack/main.cpp
+++ 
b/packages/Python/lldbsuite/test/lang/cpp/class-template-parameter-pack/main.cpp
@@ -37,7 +37,7 @@ template <> struct D : D {
 
 int main (int argc, char const *argv[])
 {
-C myC;
+C myC; //% self.runCmd("settings set 
target.experimental.inject-local-vars false")
 C myLesserC;
 myC.member = 64;
 (void)C().isSixteenThirtyTwo();

The test does things like invoke methods on temporary template objects:
//% self.expect("expression -- C().isSixteenThirtyTwo()", 
DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ["false"])

The above expression currently works because there’s a local of type C. With injected locals, the type is made readily available to Clang. No type 
lookup is required for this to work in this setup.

If you stop injecting locals, the test fails. We don’t provide the information 
to Clang to understand what C is. The reason is that when Clang parses “C”, it is going to ask about “C”, not the fully templated name. Our 
accelerator tables contain references to the full names, but not to C alone and 
we never find it. If I change Clang and dsymutil to add an accelerator for “C” 
each time an instance of C is seen then it nearly works. I just need this 
additional lldb patch:
diff --git a/source/Symbol/TypeMap.cpp b/source/Symbol/TypeMap.cpp
index 2838039ad..d2f2026bf 100644
--- a/source/Symbol/TypeMap.cpp
+++ b/source/Symbol/TypeMap.cpp
@@ -227,8 +227,11 @@ void TypeMap::RemoveMismatchedTypes(const std::string 
_scope,
   } else {
 // The type we are currently looking at doesn't exists in a namespace
 // or class, so it only matches if there is no type scope...
-keep_match =
-type_scope.empty() && type_basename.compare(match_type_name) == 0;
+if (type_scope.empty()) {
+  keep_match = type_basename.compare(match_type_name) == 0 ||
+(strlen(match_type_name) > type_basename.size() &&
+ match_type_name[type_basename.size()] == '<');
+}
   }
 }

I didn’t post this as a Phabricator review as it requires changes in llvm 
before doing anything in LLDB and I wanted to make sure we agree this is the 
right thing to do. I’m also not sure if this works out of the box on platforms 
without accelerator tables.

WDYT?

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

Re: [Lldb-commits] [lldb] r331049 - Always normalize FileSpec paths.

[Frédéric Riss via lldb-commits]

> On Apr 27, 2018, at 11:17 AM, Jim Ingham via lldb-commits 
>  wrote:
> 
> Greg,
> 
> Your new FileSpecTest unit tests are failing in the Xcode build of lldb, e.g.:
> 
> http://lab.llvm.org:8080/green/view/LLDB/job/lldb-xcode/6271/consoleFull#-1083450927b825e790-484f-4586-af29-73c4754ff671
> 
> Can you figure out what's up with this?

Once you get past the unit test, it looks like it also broke 
TestBreakpointCommand.py. Please fix this quickly.

Fred

> Jim
> 
> BTW, the "reply to" for lldb-commits mails for you is still your apple.com 
> address.  I don't know where that gets set but you should probably update 
> that at some point.
> 
> 
> 
> 
>> On Apr 27, 2018, at 8:45 AM, Greg Clayton via lldb-commits 
>>  wrote:
>> 
>> Author: gclayton
>> Date: Fri Apr 27 08:45:58 2018
>> New Revision: 331049
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=331049=rev
>> Log:
>> Always normalize FileSpec paths.
>> 
>> Always normalizing lldb_private::FileSpec paths will help us get a 
>> consistent results from comparisons when setting breakpoints and when 
>> looking for source files. This also removes a lot of complexity from the 
>> comparison routines. Modified the DWARF line table parser to use the 
>> normalized compile unit directory if needed.
>> 
>> Differential Revision: https://reviews.llvm.org/D45977
>> 
>> 
>> Modified:
>> lldb/trunk/include/lldb/Core/FileSpecList.h
>> lldb/trunk/include/lldb/Utility/FileSpec.h
>> lldb/trunk/source/Breakpoint/BreakpointResolverFileLine.cpp
>> lldb/trunk/source/Core/FileSpecList.cpp
>> lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
>> lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp
>> lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugLine.h
>> lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
>> lldb/trunk/source/Symbol/CompileUnit.cpp
>> lldb/trunk/source/Symbol/Declaration.cpp
>> lldb/trunk/source/Utility/FileSpec.cpp
>> lldb/trunk/unittests/Utility/FileSpecTest.cpp
>> 
>> Modified: lldb/trunk/include/lldb/Core/FileSpecList.h
>> URL: 
>> http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Core/FileSpecList.h?rev=331049=331048=331049=diff
>> ==
>> --- lldb/trunk/include/lldb/Core/FileSpecList.h (original)
>> +++ lldb/trunk/include/lldb/Core/FileSpecList.h Fri Apr 27 08:45:58 2018
>> @@ -119,16 +119,11 @@ public:
>> /// @param[in] full
>> /// Should FileSpec::Equal be called with "full" true or false.
>> ///
>> -  /// @param[in] remove_backup_dots
>> -  /// Should FileSpec::Equal be called with "remove_backup_dots" true or
>> -  /// false.
>> -  ///
>> /// @return
>> /// The index of the file that matches \a file if it is found,
>> /// else UINT32_MAX is returned.
>> //--
>> -  size_t FindFileIndex(size_t idx, const FileSpec , bool full,
>> -   bool remove_backup_dots = false) const;
>> +  size_t FindFileIndex(size_t idx, const FileSpec , bool full) const;
>> 
>> //--
>> /// Get file at index.
>> 
>> Modified: lldb/trunk/include/lldb/Utility/FileSpec.h
>> URL: 
>> http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Utility/FileSpec.h?rev=331049=331048=331049=diff
>> ==
>> --- lldb/trunk/include/lldb/Utility/FileSpec.h (original)
>> +++ lldb/trunk/include/lldb/Utility/FileSpec.h Fri Apr 27 08:45:58 2018
>> @@ -256,8 +256,7 @@ public:
>> //--
>> static int Compare(const FileSpec , const FileSpec , bool full);
>> 
>> -  static bool Equal(const FileSpec , const FileSpec , bool full,
>> -bool remove_backups = false);
>> +  static bool Equal(const FileSpec , const FileSpec , bool full);
>> 
>> //--
>> /// Case sensitivity of path.
>> @@ -488,12 +487,6 @@ public:
>> size_t MemorySize() const;
>> 
>> //--
>> -  /// Normalize a pathname by collapsing redundant separators and
>> -  /// up-level references.
>> -  //--
>> -  FileSpec GetNormalizedPath() const;
>> -
>> -  //--
>> /// Change the file specified with a new path.
>> ///
>> /// Update the contents of this object with a new path. The path will
>> 
>> Modified: lldb/trunk/source/Breakpoint/BreakpointResolverFileLine.cpp
>> URL: 
>> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Breakpoint/BreakpointResolverFileLine.cpp?rev=331049=331048=331049=diff
>> ==
>> --- 

Re: [Lldb-commits] [lldb] r330066 - [debugserver] Remove dead function call. NFCI.

[Frédéric Riss via lldb-commits]

> On Apr 13, 2018, at 1:47 PM, Davide Italiano via lldb-commits 
>  wrote:
> 
> Author: davide
> Date: Fri Apr 13 13:47:25 2018
> New Revision: 330066
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=330066=rev
> Log:
> [debugserver] Remove dead function call. NFCI.
> 
> Modified:
>lldb/trunk/tools/debugserver/source/RNBRemote.cpp
> 
> Modified: lldb/trunk/tools/debugserver/source/RNBRemote.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/debugserver/source/RNBRemote.cpp?rev=330066=330065=330066=diff
> ==
> --- lldb/trunk/tools/debugserver/source/RNBRemote.cpp (original)
> +++ lldb/trunk/tools/debugserver/source/RNBRemote.cpp Fri Apr 13 13:47:25 2018
> @@ -6089,9 +6089,6 @@ rnb_err_t RNBRemote::HandlePacket_qProce
>   ((addr_size == 8) ? sizeof(mach_header_64) : sizeof(mach_header));
>   load_command lc;
>   for (uint32_t i = 0; i < mh.ncmds && !os_handled; ++i) {
> -const nub_size_t bytes_read =
> -DNBProcessMemoryRead(pid, load_command_addr, sizeof(lc), );
> -
> uint32_t major_version, minor_version, patch_version;
> auto *platform = DNBGetDeploymentInfo(pid, lc, load_command_addr,
>   major_version, minor_version,

This is not dead, it fills in lc which is used on the next line. I doubt this 
passes tests if you use the built debug server. The return value is unused 
though.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [lldb] r327356 - [ExpressionParser] Fix crash when evaluating invalid expresssions.

[Frédéric Riss via lldb-commits]

> On Mar 12, 2018, at 6:40 PM, Davide Italiano via lldb-commits 
>  wrote:
> 
> Author: davide
> Date: Mon Mar 12 18:40:00 2018
> New Revision: 327356
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=327356=rev
> Log:
> [ExpressionParser] Fix crash when evaluating invalid expresssions.
> 
> Typical example, illformed comparisons (operator== where LHS and
> RHS are not compatible). If a symbol matched `operator==` in any
> of the object files lldb inserted a generic function declaration
> in the ASTContext on which Sema operates. Maintaining the AST
> context invariants is fairly tricky and sometimes resulted in
> crashes inside clang (or assertions hit).
> 
> The real reason why this feature exists in the first place is
> that of allowing users to do something like:
> (lldb) call printf("patatino")
> 
> even if the debug informations for printf() is not available.
> Eventually, we might reconsider this feature in its
> entirety, but for now we can't remove it as it would break
> a bunch of users. Instead, try to limit it to non-C++ symbols,
> where getting the invariants right is hopefully easier.
> 
> Now you can't do in lldb anymore
> (lldb) call _Zsomethingsomething(1,2,3)
> 
> but that doesn't seem to be such a big loss.

I’m somewhat surprised by this. My understanding of the crash you were 
investigating is that Clang crashed because we injected a Decl looking like 
this: “void operator==(…)” after finding the operator== symbol somewhere. I 
think injecting bogus C++ Decls makes no sense and it cannot really work anyway.

On the other hand, I see no harm in injecting “_Zsomethingsomething(…)” as a C 
Decl. This can be useful, and people should be able to call raw symbols in 
their binaries. Is there no way to keep the later while preventing the creation 
of broken C++ decls?

Fred

> 
> 
> Added:
>lldb/trunk/lit/Expr/Inputs/basic.cpp
>lldb/trunk/lit/Expr/TestCallCppSym.test
> Modified:
>lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
> 
> Added: lldb/trunk/lit/Expr/Inputs/basic.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Expr/Inputs/basic.cpp?rev=327356=auto
> ==
> --- lldb/trunk/lit/Expr/Inputs/basic.cpp (added)
> +++ lldb/trunk/lit/Expr/Inputs/basic.cpp Mon Mar 12 18:40:00 2018
> @@ -0,0 +1,12 @@
> +class Patatino {
> +private:
> +  long tinky;
> +
> +public:
> +  Patatino(long tinky) { this->tinky = tinky; }
> +};
> +
> +int main(void) {
> +  Patatino *a = new Patatino(26);
> +  return 0;
> +}
> 
> Added: lldb/trunk/lit/Expr/TestCallCppSym.test
> URL: 
> http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Expr/TestCallCppSym.test?rev=327356=auto
> ==
> --- lldb/trunk/lit/Expr/TestCallCppSym.test (added)
> +++ lldb/trunk/lit/Expr/TestCallCppSym.test Mon Mar 12 18:40:00 2018
> @@ -0,0 +1,6 @@
> +# RUN: %cxx %p/Inputs/basic.cpp -g -o %t && %lldb -b -s %s -- %t 2>&1 | 
> FileCheck %s
> +
> +breakpoint set --file basic.cpp --line 12
> +run
> +call (int)_Znwm(23)
> +# CHECK: error: use of undeclared identifier '_Znwm'
> 
> Modified: 
> lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp?rev=327356=327355=327356=diff
> ==
> --- 
> lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp 
> (original)
> +++ 
> lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp 
> Mon Mar 12 18:40:00 2018
> @@ -2072,6 +2072,15 @@ void ClangExpressionDeclMap::AddOneFunct
>   return;
> }
>   } else if (symbol) {
> +// Don't insert a generic function decl for C++ symbol names.
> +// Creating a generic function decl is almost surely going to cause 
> troubles
> +// as it breaks Clang/Sema invariants and causes crashes in clang while
> +// we're trying to evaluate the expression.
> +// This means users can't call C++ functions by mangled name when there
> +// are no debug info (as it happens for C symbol, e.g. printf()).
> +if (CPlusPlusLanguage::IsCPPMangledName(
> +symbol->GetMangled().GetMangledName().GetCString()))
> +  return;
> fun_address = symbol->GetAddress();
> function_decl = context.AddGenericFunDecl();
> is_indirect_function = symbol->IsIndirect();
> 
> 
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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