Re: [Lldb-commits] [PATCH] D13066: DWARFASTParserClang::CompleteTypeFromDWARF: Handle incomplete baseclass or child
zturner added a comment. To be clear, I would like this Makefile to turn into the following: LEVEL = ../../../make CXX_SOURCES = main.cpp length.cpp DEBUG_INFO_FULL = True DEBUG_INFO_LIMITED = True And that's it. You shouldn't need anything else. Whatever needs to happen in Makefile.rules to make this work should be done. By default, DEBUG_INFO_FULL should be True and DEBUG_INFO_LIMITED should be false. This will allow all other makefiles that don't care about the debug info to work as they normally do. If neither variable is True, it won't use -g. If only DEBUG_INFO_FULL is True, it will build one target and use -fno-limit-debug-info. If only DEBUG_INFO_LIMITED is true, it will build one target and use -flimit-debug-info. If both are true, it will build two targets, one for each case. Does this seem reasonable? http://reviews.llvm.org/D13066 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D13066: DWARFASTParserClang::CompleteTypeFromDWARF: Handle incomplete baseclass or child
zturner added a comment. Also,when you say the next iteration, is this something that is going to happen at an unknown time in the future whenever you happen to touch it again, or are you working on another iteration now? http://reviews.llvm.org/D13066 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D13066: DWARFASTParserClang::CompleteTypeFromDWARF: Handle incomplete baseclass or child
Also,when you say the next iteration, is this something that is going to happen at an unknown time in the future whenever you happen to touch it again, or are you working on another iteration now? On Thu, Sep 24, 2015 at 4:25 PM Zachary Turner wrote: > To be clear, I would like this Makefile to turn into the following: > > LEVEL = ../../../make > > CXX_SOURCES = main.cpp length.cpp > > DEBUG_INFO_FULL = True > DEBUG_INFO_LIMITED = True > > And that's it. You shouldn't need anything else. Whatever needs to > happen in Makefile.rules to make this work should be done. > > By default, DEBUG_INFO_FULL should be True and DEBUG_INFO_LIMITED should > be false. This will allow all other makefiles that don't care about the > debug info to work as they normally do. If neither variable is True, it > won't use -g. If only DEBUG_INFO_FULL is True, it will build one target > and use -fno-limit-debug-info. If only DEBUG_INFO_LIMITED is true, it will > build one target and use -flimit-debug-info. If both are true, it will > build two targets, one for each case. > > Does this seem reasonable? > > On Thu, Sep 24, 2015 at 12:57 PM Zachary Turner > wrote: > >> zturner added a comment. >> >> I know, I've seen them in a few places myself. But I think we should move >> away from that, because it's a large source of portability problems >> >> >> http://reviews.llvm.org/D13066 >> >> >> >> ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D13066: DWARFASTParserClang::CompleteTypeFromDWARF: Handle incomplete baseclass or child
To be clear, I would like this Makefile to turn into the following: LEVEL = ../../../make CXX_SOURCES = main.cpp length.cpp DEBUG_INFO_FULL = True DEBUG_INFO_LIMITED = True And that's it. You shouldn't need anything else. Whatever needs to happen in Makefile.rules to make this work should be done. By default, DEBUG_INFO_FULL should be True and DEBUG_INFO_LIMITED should be false. This will allow all other makefiles that don't care about the debug info to work as they normally do. If neither variable is True, it won't use -g. If only DEBUG_INFO_FULL is True, it will build one target and use -fno-limit-debug-info. If only DEBUG_INFO_LIMITED is true, it will build one target and use -flimit-debug-info. If both are true, it will build two targets, one for each case. Does this seem reasonable? On Thu, Sep 24, 2015 at 12:57 PM Zachary Turner wrote: > zturner added a comment. > > I know, I've seen them in a few places myself. But I think we should move > away from that, because it's a large source of portability problems > > > http://reviews.llvm.org/D13066 > > > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D13066: DWARFASTParserClang::CompleteTypeFromDWARF: Handle incomplete baseclass or child
zturner added a comment. I know, I've seen them in a few places myself. But I think we should move away from that, because it's a large source of portability problems http://reviews.llvm.org/D13066 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D13066: DWARFASTParserClang::CompleteTypeFromDWARF: Handle incomplete baseclass or child
I don't want compiler options in the test Makefiles. Please move the logic to Makefile.rules, and create a variable and set that instead. On Thu, Sep 24, 2015 at 11:43 AM Greg Clayton via lldb-commits < lldb-commits@lists.llvm.org> wrote: > clayborg added a comment. > > Each shared library is an object that can be shared between multiple > targets. We do not want to injecting types from another shared library into > the static notion of what a type is within a shared library. Why? What if > one target in lldb loads liba.so which has a forward declaration to "class > Bar;", and this same target loads libb.so which contains a full definition > for "Bar". Another target also loads liba.so, but it loads a different > version of "libb.so" which has a newer more up to date version of "Bar" > because you are doing a edit/compile/debug cycle. We can't take a full > definition of "Bar" and place it into the debug info for liba.so because it > can be wrong. So each shared library always just parses the types > _as_they_see_them_ in their debug info. > > We do allow variable introspection to always grab the complete type from > another shared library when displaying the type, we just don't allow the > static notion of a type that comes from a specific shared library to be > augmented within that shared library. > > So lets say you have liba.so that contains Foo class which has a "Bar > *m_bar;" member variable. liba.so doesn't have a full definition for "Bar" > which is OK. "Bar", in liba.so, is known to be "class Bar;", nothing else. > When we are debugging a a.out binary later that loads both liba.so and > libb.so (which contains the full definition for "Bar"), in the variable > display code we see that we want to expand "Bar" in the variables view and > we note that we have a full definition of "Bar" in libb.so, so we end up > using the full definition from libb.so in place of the forward declaration. > Again, we just switch the types. We can do this because have have a type > that comes from a specific target, and within that target (a collection of > shared libraries) we can come up with the right definition for "Bar" > without modifying the type itself from the debug info within liba.so. > > So there should be not problems when viewing variables because we have > that covered. > > If we have any problems in the expression parser, we will need to solve > them in the say kind of way: given a target context, determine the right > version of the type to use and copy into the ASTContext for the expression. > We currently import types from multiple different ASTContexts into an > expression ASTContext for each expression that we run, so it would be > possible to identify any classes that were forward declarations and try to > complete them during the ASTContext -> ASTContext copy cycle. We would need > to somehow mark these incomplete types to allow them to be completed later. > Like for example the forward declaration to a base class. We used to just > complete the definition for the base class and say it had no member and no > methods. We have hooks in to allow the DWARF to assist in laying out a > class so that we can correct any alignment issues we run into (since DWARF > doesn't always capture any #pragma pack and other compiler directives that > can affect how a class is laid out), but we could allow these base classes > to be created and mark them somehow in the ClangASTContext so we can > identify them later during any expression use. We could also use this for > the Variable display code and find the full definition of the forward > declaration base class... > > So, any changes you make in future fixes must adhere to: > > - a type is parsed exactly as it is represented within the object file > itself, no outside definitions from other shared libraries can be merged > into these types > - it is fine to switch over to using a different version of a type (the > full definition of "Bar" from libb.so when you have a forward declaration > for "Bar" in liba.so) when displaying variables or evaluating expressions > that have a Target in their execution context since the target has a list > of all the shared libraries that are currently loaded. This allows process > a.out to load one version of "Bar" from libb.so, and process b.out to load > another version of "Bar" from libc.so > - We can modify the ClangASTContext ASTImporter to do the same kind of > type switching when copying types into the expression ASTContext > > > http://reviews.llvm.org/D13066 > > > > ___ > 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
Re: [Lldb-commits] [PATCH] D13066: DWARFASTParserClang::CompleteTypeFromDWARF: Handle incomplete baseclass or child
clayborg added a comment. Each shared library is an object that can be shared between multiple targets. We do not want to injecting types from another shared library into the static notion of what a type is within a shared library. Why? What if one target in lldb loads liba.so which has a forward declaration to "class Bar;", and this same target loads libb.so which contains a full definition for "Bar". Another target also loads liba.so, but it loads a different version of "libb.so" which has a newer more up to date version of "Bar" because you are doing a edit/compile/debug cycle. We can't take a full definition of "Bar" and place it into the debug info for liba.so because it can be wrong. So each shared library always just parses the types _as_they_see_them_ in their debug info. We do allow variable introspection to always grab the complete type from another shared library when displaying the type, we just don't allow the static notion of a type that comes from a specific shared library to be augmented within that shared library. So lets say you have liba.so that contains Foo class which has a "Bar *m_bar;" member variable. liba.so doesn't have a full definition for "Bar" which is OK. "Bar", in liba.so, is known to be "class Bar;", nothing else. When we are debugging a a.out binary later that loads both liba.so and libb.so (which contains the full definition for "Bar"), in the variable display code we see that we want to expand "Bar" in the variables view and we note that we have a full definition of "Bar" in libb.so, so we end up using the full definition from libb.so in place of the forward declaration. Again, we just switch the types. We can do this because have have a type that comes from a specific target, and within that target (a collection of shared libraries) we can come up with the right definition for "Bar" without modifying the type itself from the debug info within liba.so. So there should be not problems when viewing variables because we have that covered. If we have any problems in the expression parser, we will need to solve them in the say kind of way: given a target context, determine the right version of the type to use and copy into the ASTContext for the expression. We currently import types from multiple different ASTContexts into an expression ASTContext for each expression that we run, so it would be possible to identify any classes that were forward declarations and try to complete them during the ASTContext -> ASTContext copy cycle. We would need to somehow mark these incomplete types to allow them to be completed later. Like for example the forward declaration to a base class. We used to just complete the definition for the base class and say it had no member and no methods. We have hooks in to allow the DWARF to assist in laying out a class so that we can correct any alignment issues we run into (since DWARF doesn't always capture any #pragma pack and other compiler directives that can affect how a class is laid out), but we could allow these base classes to be created and mark them somehow in the ClangASTContext so we can identify them later during any expression use. We could also use this for the Variable display code and find the full definition of the forward declaration base class... So, any changes you make in future fixes must adhere to: - a type is parsed exactly as it is represented within the object file itself, no outside definitions from other shared libraries can be merged into these types - it is fine to switch over to using a different version of a type (the full definition of "Bar" from libb.so when you have a forward declaration for "Bar" in liba.so) when displaying variables or evaluating expressions that have a Target in their execution context since the target has a list of all the shared libraries that are currently loaded. This allows process a.out to load one version of "Bar" from libb.so, and process b.out to load another version of "Bar" from libc.so - We can modify the ClangASTContext ASTImporter to do the same kind of type switching when copying types into the expression ASTContext http://reviews.llvm.org/D13066 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D13066: DWARFASTParserClang::CompleteTypeFromDWARF: Handle incomplete baseclass or child
sivachandra added inline comments. Comment at: test/lang/cpp/incomplete-types/Makefile:8 @@ +7,3 @@ + +ifneq (,$(findstring clang,$(CC))) + CFLAGS_LIMIT += -flimit-debug-info dblaikie wrote: > In case it's interesting, you can get similarly problematic DWARF by using a > dynamic class (one with virtual functions) with a key function that is not > defined in the current translation unit. > > GCC implements this behavior as well (whereas GCC doesn't implement the > behavior that is triggering on basic_string involving explicit instantiation > declarations/definitions) and also has a flag for it: > -femit-class-debug-always (I think last I recall, Eric Christopher mentioned > he'd looked at the GCC implementation of this flag and it differed in some > ways from Clang's, so he was reticent to add a compatibility flag for this in > Clang... but we could discuss/revisit that, perhaps (though I suppose it > wouldn't allow you to /enable/ this optimization, only disable it - not sure > if there's a way to opt-in in GCC)) > > Though it's easy to "disable" the feature by simply not triggering it, rather > than using a flag to turn it off - eg: providing no key function for a type > (if you're triggering the dynamic class case, not the template case), or > removing an explicit instantiation declaration/definition (if you're > triggering the template case) Thanks for this info. I can remove the use of std::string and make use of a dynamic class to test this behavior. And, if it can be tested with GCC as well, then @skipIfGcc can be removed. Comment at: test/lang/cpp/incomplete-types/TestCppIncompleteTypes.py:5 @@ +4,3 @@ + +class TestCppIncompleteTypes(TestBase): + dblaikie wrote: > You don't seem to have a test case for the shared library case in the bug > report? (decl in one obj, def in the other, shared library/object/whatever > boundary between the two) > > (& not sure what the behavior is in the case where the def is provided in one > obj and they are statically linked together - bug report mentions the failure > doesn't occur, but I don't know if it does the right thing (actually finds > the definition) - I know lldb used to not do the right thing there, and had > some error message about the compiler being wrong) Per my understanding, LLDB does not search the world to complete or lookup a type. So, shared library case will not still work. If the objects are statically linked, it will however. Making it work in the shared library case is the next step. I treated pr24812 as unrelated to this. I can open it back if you feel it should be kept open until the shared library case is fixed. http://reviews.llvm.org/D13066 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D13066: DWARFASTParserClang::CompleteTypeFromDWARF: Handle incomplete baseclass or child
dblaikie added a subscriber: dblaikie. Comment at: test/lang/cpp/incomplete-types/Makefile:8 @@ +7,3 @@ + +ifneq (,$(findstring clang,$(CC))) + CFLAGS_LIMIT += -flimit-debug-info In case it's interesting, you can get similarly problematic DWARF by using a dynamic class (one with virtual functions) with a key function that is not defined in the current translation unit. GCC implements this behavior as well (whereas GCC doesn't implement the behavior that is triggering on basic_string involving explicit instantiation declarations/definitions) and also has a flag for it: -femit-class-debug-always (I think last I recall, Eric Christopher mentioned he'd looked at the GCC implementation of this flag and it differed in some ways from Clang's, so he was reticent to add a compatibility flag for this in Clang... but we could discuss/revisit that, perhaps (though I suppose it wouldn't allow you to /enable/ this optimization, only disable it - not sure if there's a way to opt-in in GCC)) Though it's easy to "disable" the feature by simply not triggering it, rather than using a flag to turn it off - eg: providing no key function for a type (if you're triggering the dynamic class case, not the template case), or removing an explicit instantiation declaration/definition (if you're triggering the template case) Comment at: test/lang/cpp/incomplete-types/TestCppIncompleteTypes.py:5 @@ +4,3 @@ + +class TestCppIncompleteTypes(TestBase): + You don't seem to have a test case for the shared library case in the bug report? (decl in one obj, def in the other, shared library/object/whatever boundary between the two) (& not sure what the behavior is in the case where the def is provided in one obj and they are statically linked together - bug report mentions the failure doesn't occur, but I don't know if it does the right thing (actually finds the definition) - I know lldb used to not do the right thing there, and had some error message about the compiler being wrong) Comment at: test/lang/cpp/incomplete-types/length.h:4 @@ +3,3 @@ + +#include + Having a test case depend on the entire header of seems a bit brittle - and means this test may or may not test the behavior you're interested in, depending on how the standard library is defined (depending on whether it actually has an explicit instantiation decl/def for basic_string or not) http://reviews.llvm.org/D13066 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D13066: DWARFASTParserClang::CompleteTypeFromDWARF: Handle incomplete baseclass or child
clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Looks good as long as the test suite if happy. http://reviews.llvm.org/D13066 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D13066: DWARFASTParserClang::CompleteTypeFromDWARF: Handle incomplete baseclass or child
sivachandra added a comment. I am not sure how portable my Makefile is. I have no idea if it will work on Windows for example. http://reviews.llvm.org/D13066 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
