Re: [lldb-dev] [cfe-dev] stable layout bug for imported record decls.
Hi Gábor, Richard, Hi Richard, maybe my understanding is not correct, but I believe the > situation is somewhat twisted and reversed... Is this relationship recursive? (Should it be?) I.e. When ClangASTSource uses ASTImporter to minimally import a Decl, should that Decl have another ExternalASTSource attached (possibly the original ClangASTSource) to complete the minimally imported Decl on demand? -- Lang. On Tue, Aug 14, 2018 at 6:49 AM Gábor Márton wrote: > > It might be interesting to consider how this is addressed by the > ExternalASTSource interface. There, we have separate "import the lexical > contents of this AST node (including populating the lexical declarations > list with all members, in the right order)", "import the lookup results for > this name in this context (and cache them but don't add them to the list of > lexical members)" and "import this specific declaration member (but don't > add it to the list of lexical members)" queries. One could imagine doing > something similar for the AST importer: when you're performing a minimal > import but you want to import a method declaration, don't insert it into > the list of lexical members of the enclosing record. Instead, defer doing > that until a complete type is required (at that point, you'd need to import > all the relevant members anyway, including the base classes and virtual > functions in the right order). > > Hi Richard, maybe my understanding is not correct, but I believe the > situation is somewhat twisted and reversed. > Seems like LLDB already exercises the ExternalASTSource interface. The > purpose of using it is exactly to load the external lexical contents > of an AST node (as you suggested). However, the load is implemented by > the means of the ASTImporter (in minimal mode). Consider the attached > function call trace: > When the user instructs LLDB to evaluate an expression then LLDB > exercises the parser `clang::ParseAST`, which in turn does a lookup > `clang::DeclContext::lookup`. During the lookup, > `lldb_private::ClangASTSource::FindExternalVisibleDeclsByName` is > called (this function overloads > `ExternalASTSource::FindExternalVisibleDeclsByName`). > And finally, through `lldb_private::ClangASTImporter::CopyType` we end > up in `clang::ASTImporter::Import`. > > Gabor > > ``` > #4 0x741a72ed in clang::ASTNodeImporter::ImportDeclParts > (this=this@entry=0x7fff8be0, D=D@entry=0x632038, > DC=@0x7fff8ac8: 0x0, > LexicalDC=@0x7fff8ad0: 0x0, Name=..., ToD=@0x7fff8ad8: 0x0, > Loc=...) > at > /home/gerazo/repos/codechecker_dev_env/CTU/llvm/tools/clang/lib/AST/ASTImporter.cpp:1062 > #5 0x741a8d55 in clang::ASTNodeImporter::VisitRecordDecl > (this=0x7fff8be0, D=0x632038) > at > /home/gerazo/repos/codechecker_dev_env/CTU/llvm/tools/clang/lib/AST/ASTImporter.cpp:2126 > #6 0x7419377b in clang::ASTImporter::Import (this=0x70cc90, > FromD=0x632038) > at > /home/gerazo/repos/codechecker_dev_env/CTU/llvm/tools/clang/lib/AST/ASTImporter.cpp:7054 > #7 0x741939f7 in clang::ASTNodeImporter::VisitRecordType > (this=0x7fff8c40, T=) > at > /home/gerazo/repos/codechecker_dev_env/CTU/llvm/tools/clang/lib/AST/ASTImporter.cpp:851 > #8 0x74196f65 in clang::TypeVisitor clang::QualType>::Visit (this=this@entry=0x7fff8c40, T= out>) > at > /home/gerazo/repos/codechecker_dev_env/CTU/llvm/tools/clang/include/clang/AST/TypeNodes.def:92 > #9 0x74197187 in clang::ASTImporter::Import (this=0x70cc90, > FromT=FromT@entry=...) > at > /home/gerazo/repos/codechecker_dev_env/CTU/llvm/tools/clang/lib/AST/ASTImporter.cpp:6983 > #10 0x77351c92 in lldb_private::ClangASTImporter::CopyType > (this=, dst_ast=, src_ast= out>, type=type@entry=...) > at > /home/gerazo/repos/codechecker_dev_env/CTU/llvm/tools/lldb/source/Symbol/ClangASTImporter.cpp:61 > #11 0x774bdb95 in > lldb_private::ClangASTSource::GuardedCopyType > (this=this@entry=0x611e20, src_type=...) > at > /home/gerazo/repos/codechecker_dev_env/CTU/llvm/tools/lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp:2040 > #12 0x774e9ffe in > lldb_private::ClangExpressionDeclMap::GetVariableValue > (this=this@entry=0x611e20, var=std::shared_ptr (count 6, weak 1) > 0x7fffe80020f0, > var_location=..., user_type=user_type@entry=0x7fff8fc0, > parser_type=parser_type@entry=0x7fff8fe0) > at > /home/gerazo/repos/codechecker_dev_env/CTU/llvm/tools/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:1627 > #13 0x774ea3a7 in > lldb_private::ClangExpressionDeclMap::AddOneVariable > (this=this@entry=0x611e20, context=..., > var=std::shared_ptr (count 6, weak 1) 0x7fffe80020f0, valobj=..., > current_id=current_id@entry=2) > at > /home/gerazo/repos/codechecker_dev_env/CTU/llvm/tools/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:1679 > #14 0x774ec50b in > lldb_private::ClangExpressionDeclMap::Fin
Re: [lldb-dev] [cfe-dev] stable layout bug for imported record decls.
Hi Richard,
Perhaps a better approach would be to make the "minimal" mode for the
> ASTImporter provide an ExternalASTSource to lazily complete the AST as
> needed (thereby avoiding violating the invariant, because you would
> populate the lexical declarations list whenever anyone actually asks for
> it).
This seems worth investigating. I wonder if it might also fix another bug
that I know of involving virtual methods with covariant return types. (You
and I actually discussed it at one of the socials a while back, but I have
not had time to dig into it further.)
The reproducer for the bug is:
class Foo {};
class Bar : public Foo {};
class Base {
public:
virtual Foo* foo() { return nullptr; }
};
class Derived : public Base {
public:
virtual Bar* foo() { return nullptr; }
};
int main() {
Derived D;
D.foo(); // Evaluate 'D.foo()' here, crash LLDB.
}
The issue is that since Bar's definition is not used its bases are never
imported, and so the call to Bar::bases() crashes in CodeGen. If we
provided an ExternalASTSource, would that be able to lazily complete the
bases?
Cheers,
Lang.
On Mon, Aug 13, 2018 at 3:30 PM Richard Smith wrote:
> On Thu, 9 Aug 2018 at 10:47, Lang Hames via cfe-dev <
> cfe-...@lists.llvm.org> wrote:
>
>> Hi clang-dev, lldb-dev,
>>
>> It looks like my clang commit r305850, which modified ASTImporter to
>> import method override tables from an external context, introduced a new
>> bug which manifests as incorrect vtable layouts for LLDB expression code.
>>
>> The bug itself is fairly straightforward. In r305850 I introduced the
>> following method, which is called from ASTNodeImporter::VisitFunctionDecl:
>>
>> void ASTNodeImporter::ImportOverrides(CXXMethodDecl *ToMethod,
>> CXXMethodDecl *FromMethod) {
>> for (auto *FromOverriddenMethod : FromMethod->overridden_methods())
>> ToMethod->addOverriddenMethod(
>> cast(Importer.Import(const_cast(
>> FromOverriddenMethod;
>> }
>>
>> This will produce the correct override table, but can also cause methods
>> in the Base class to be visited in the wrong order. Consider:
>>
>> class Base {
>> public:
>> virtual void bar() {}
>> virtual void foo() {}
>> };
>>
>> class Derived : public Base {
>> public:
>> void foo() override {}
>> };
>>
>> If Derived is imported into a new context before Base, then the importer
>> will visit Derived::foo, and (via ImportOverrides) immediately import
>> Base::foo, but this happens before Base::bar is imported. As a consequence,
>> the decl order on the imported Base class will end up being [ foo, bar ],
>> instead of [ bar, foo ]. In LLDB expression evaluation this manifests as an
>> incorrect vtable layout for Base, with foo occupying the first slot.
>>
>> I am looking for suggestions on the right way to fix this. A brute force
>> solution might be to always have ASTNodeImporter::VisitRecordDecl visit all
>> base classes, then all virtual methods, which would ensure they are visited
>> in the original decl order. However I do not know whether this covers all
>> paths by which a CXXRecordDecl might be imported, nor whether the
>> performance of this solution would be acceptable (it seems like it would
>> preclude a lot of laziness).
>>
>> Alternatively we might be able to associate an index with each imported
>> decl and sort on that when we complete the type, but that would leave
>> imported decls in the wrong order until the type was complete, and since I
>> do not know all the use cases for the importer I'm concerned people may
>> rely on the decl order before type is complete.
>>
>> Any insight from ASTImporter experts would be greatly appreciated. :)
>>
>
> Hi Lang,
>
> It might be interesting to consider how this is addressed by the
> ExternalASTSource interface. There, we have separate "import the lexical
> contents of this AST node (including populating the lexical declarations
> list with all members, in the right order)", "import the lookup results for
> this name in this context (and cache them but don't add them to the list of
> lexical members)" and "import this specific declaration member (but don't
> add it to the list of lexical members)" queries. One could imagine doing
> something similar for the AST importer: when you're performing a minimal
> import but you want to import a method declaration, don't insert it into
> the list of lexical members of the enclosing record. Instead, defer doing
> that until a complete type is required (at that point, you'd need to import
> all the relevant members anyway, including the base classes and virtual
> functions in the right order).
>
> The above approach would violate AST invariants (you'd have a declaration
> whose lexical parent doesn't believe it lexically contains the child), but
> I don't know off-hand whether that would be problematic. Perhaps a better
> approach would be to make the "minimal" mode for the ASTImporter provide an
>
Re: [lldb-dev] stable layout bug for imported record decls.
gt; Hi Lang,
> > >
> > > We faced a very similar issue with record fields where import order
> can change the order of imported FieldDecls resulting in broken
> ASTRecordLayout. The patch for this issue is on review:
> https://reviews.llvm.org/D44100. It just reorders the fields after
> structure import is finished. CSA guys also reported the same problem with
> FriendDecls in the same review.The order of methods was not a problem for
> us but your report adds a new item to support. It looks like _all_ decls
> inside RecordDecl have to be reordered. I'll try to resurrect the patch
> this weekend (it is a bit outdated due to my workload, sorry) and add you
> as a reviewer so you can check if it solves the problem or not.
> > >
> > > 09.08.2018 20:46, Lang Hames via lldb-dev пишет:
> > >
> > > Hi clang-dev, lldb-dev,
> > >
> > > It looks like my clang commit r305850, which modified ASTImporter to
> import method override tables from an external context, introduced a new
> bug which manifests as incorrect vtable layouts for LLDB expression code.
> > >
> > > The bug itself is fairly straightforward. In r305850 I introduced the
> following method, which is called from ASTNodeImporter::VisitFunctionDecl:
> > >
> > > void ASTNodeImporter::ImportOverrides(CXXMethodDecl *ToMethod,
> > > CXXMethodDecl *FromMethod) {
> > > for (auto *FromOverriddenMethod : FromMethod->overridden_methods())
> > > ToMethod->addOverriddenMethod(
> > > cast(Importer.Import(const_cast(
> > > FromOverriddenMethod;
> > > }
> > >
> > > This will produce the correct override table, but can also cause
> methods in the Base class to be visited in the wrong order. Consider:
> > >
> > > class Base {
> > > public:
> > > virtual void bar() {}
> > > virtual void foo() {}
> > > };
> > >
> > > class Derived : public Base {
> > > public:
> > > void foo() override {}
> > > };
> > >
> > > If Derived is imported into a new context before Base, then the
> importer will visit Derived::foo, and (via ImportOverrides) immediately
> import Base::foo, but this happens before Base::bar is imported. As a
> consequence, the decl order on the imported Base class will end up being [
> foo, bar ], instead of [ bar, foo ]. In LLDB expression evaluation this
> manifests as an incorrect vtable layout for Base, with foo occupying the
> first slot.
> > >
> > > I am looking for suggestions on the right way to fix this. A brute
> force solution might be to always have ASTNodeImporter::VisitRecordDecl
> visit all base classes, then all virtual methods, which would ensure they
> are visited in the original decl order. However I do not know whether this
> covers all paths by which a CXXRecordDecl might be imported, nor whether
> the performance of this solution would be acceptable (it seems like it
> would preclude a lot of laziness).
> > >
> > > Alternatively we might be able to associate an index with each
> imported decl and sort on that when we complete the type, but that would
> leave imported decls in the wrong order until the type was complete, and
> since I do not know all the use cases for the importer I'm concerned people
> may rely on the decl order before type is complete.
> > >
> > > Any insight from ASTImporter experts would be greatly appreciated. :)
> > >
> > > Cheers,
> > > Lang.
> > >
> > >
> > > ___
> > > lldb-dev mailing list
> > > lldb-dev@lists.llvm.org
> > > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
> > >
> > >
>
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
[lldb-dev] stable layout bug for imported record decls.
Hi clang-dev, lldb-dev,
It looks like my clang commit r305850, which modified ASTImporter to import
method override tables from an external context, introduced a new bug which
manifests as incorrect vtable layouts for LLDB expression code.
The bug itself is fairly straightforward. In r305850 I introduced the
following method, which is called from ASTNodeImporter::VisitFunctionDecl:
void ASTNodeImporter::ImportOverrides(CXXMethodDecl *ToMethod,
CXXMethodDecl *FromMethod) {
for (auto *FromOverriddenMethod : FromMethod->overridden_methods())
ToMethod->addOverriddenMethod(
cast(Importer.Import(const_cast(
FromOverriddenMethod;
}
This will produce the correct override table, but can also cause methods in
the Base class to be visited in the wrong order. Consider:
class Base {
public:
virtual void bar() {}
virtual void foo() {}
};
class Derived : public Base {
public:
void foo() override {}
};
If Derived is imported into a new context before Base, then the importer
will visit Derived::foo, and (via ImportOverrides) immediately import
Base::foo, but this happens before Base::bar is imported. As a consequence,
the decl order on the imported Base class will end up being [ foo, bar ],
instead of [ bar, foo ]. In LLDB expression evaluation this manifests as an
incorrect vtable layout for Base, with foo occupying the first slot.
I am looking for suggestions on the right way to fix this. A brute force
solution might be to always have ASTNodeImporter::VisitRecordDecl visit all
base classes, then all virtual methods, which would ensure they are visited
in the original decl order. However I do not know whether this covers all
paths by which a CXXRecordDecl might be imported, nor whether the
performance of this solution would be acceptable (it seems like it would
preclude a lot of laziness).
Alternatively we might be able to associate an index with each imported
decl and sort on that when we complete the type, but that would leave
imported decls in the wrong order until the type was complete, and since I
do not know all the use cases for the importer I'm concerned people may
rely on the decl order before type is complete.
Any insight from ASTImporter experts would be greatly appreciated. :)
Cheers,
Lang.
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
Re: [lldb-dev] Renaming lldb_private::Error
Leaving 'Status' aside for now (the rename makes perfect sense), I'm basing
my ErrorAnd / WithError naming suggestion on this comment:
Is there any chance of introducing something like make_status() into
> llvm/Error.h, that constructs the llvm::Error in such a way that it still
> interoperates nicely with everything else?
which contains a fundamental tension: Error's purpose is to be
un-ignorable, which could be considered "not nice", and is definitely at
odds with the idea of the user implicitly ignoring it if they want to
(though it can be explicitly ignored by calling consumeError).
But if it does need to be handled (and as such is called an error), then
> I'm not sure if it makes sense to say there's also a value. So ErrorOr, or
> Expected seems to convey that meaning in the only way possible. If you
> don't get the thing you're expected to get, you need to handle the error.
This is an aside from the LLDB conversation, but worth noting: While Error
instances must be dealt with, that doesn't mean Error is only useful for
hard errors. Being good for diagnostics was part of its original design
brief. The ErrorAnd concept comes into play any time you have a data
structure that can be partially malformed but still useful. Consider
libObject, for example: It should be able to parse partially malformed
object files (ones that have been truncated, or contain bad symbol indexes,
or malformed symbols, etc.). However you want to make sure that the client
explicitly acknowledges any errors in the file before proceeding (that way
they can't claim later that you didn't warn them. ;). ErrorAnd
is a perfect fit for this. It would take an Error (which may be success, a
singleton Error, or a compound Error) and an ObjectFile, and force you to
consume the Errors before accessing the file:
In pseudo-c++:
// Parse my object file.
auto ErrorAnd ObjAndErr = parseObjectFile(...);
// I claim that I'm willing to handle truncated objects, or objects
// containing bad symbol indexes. If the object file contains errors
// other than this I will bail out.
if (auto RemainingErrors = handleErrors(ObjAndErr.takeError(),
[](const BadSymbolIndex &BSI) { ...
},
[](const TruncatedObject &TO) { ...
}))
return RemainingErrors;
// Ok, *now* I can access my object:
auto Obj = ObjAndError.takeValue();
Again, I'm not recommending this for any specific LLDB interfaces (I don't
know them well enough yet), but I believe it has its place as an idiom.
Right. We know that at some point (at the least where they escape to the
> SB API's) we'll have to have a container for the content of the error which
> carries none of the programmatic imperatives we want to impose at lower
> layers.
The programmatic imperative is the key difference here.
Actually, Zachary - it's just occurred to me that that's what you may have
been asking for: A type that's structured like Error, but without the hard
requirement that it be checked? If so you're right - that might be an
interesting thing to add to llvm/Error.h.
Instead, you could do this nicely by making a Status object that can
> accumulate (and by doing so check) the errors from its primitive
> operations. You'd also have to be able to mark the Status as Succeeding.
> Then the composite operation could turn the Status back into an Error, or
> pass it out as a Status, depending on its contract. So I think it is
> worthwhile keep two separate entities.
For a function that returns an Error, and whose callees return Error, I
think you could just stay in Error mode the whole way through. Error
supports compound values, and there are idioms for accumulating them with
arbitrary checking applied at the accumulation point. Having an easy
conversion utility between Error and Status is definitely a must though.
Cheers,
Lang.
On Wed, May 10, 2017 at 7:36 PM, Jim Ingham wrote:
>
> > On May 10, 2017, at 6:28 PM, Zachary Turner wrote:
> >
> > Yes, this is just the rename.
> >
> > Regarding the naming, if you call it ErrorAnd, or WithError, or anything
> that includes the word error, you are implying that something actually went
> wrong. I don't think that's the intended use case, or at least not what I
> have in mind (and from previous conversations on the list, I don't think
> what Jim had in mind either).
> >
> > If we're going to say that something does not need to be handled, I
> don't know if we should be calling it an error at all. By definition, we
> should assert that errors must be handled, so the converse is that if it
> doesn't need to be handled, it's not an error.
> >
> > But if it does need to be handled (and as such is called an error), then
> I'm not sure if it makes sense to say there's also a value. So ErrorOr, or
> Expected seems to convey that meaning in the only way possible. If you
> don't get the thing you're expected to get, you need to handle the error.
> >
> > But it seemed like what we we
Re: [lldb-dev] Renaming lldb_private::Error
Cool. This is just the rename portion, right?
Sorry I didn't respond to your last message too.
I suppose, but I'm not sure ErrorAnd captures the intended meaning very
> well. In any case, that's not super important at this stage since this
> isn't on the immediate horizon.
Do you just mean that ErrorAnd isn't an especially nice name? I wasn't
entirely sure what make_status(...) was supposed to do so I assumed it
was to create a pair of an Error and a T. If that's the case,
make_with_error(T, Error) (and WithError) might be better names?
Cheers,
Lang.
On Tue, May 9, 2017 at 8:58 PM, Zachary Turner wrote:
> I'm probably going to be looking at submitting this this week, more likely
> sooner rather than later. If I can get it all working hopefully even
> tomorrow.
>
> On Mon, May 1, 2017 at 5:49 PM Zachary Turner wrote:
>
>> I suppose, but I'm not sure ErrorAnd captures the intended meaning very
>> well. In any case, that's not super important at this stage since this
>> isn't on the immediate horizon.
>>
>> On Mon, May 1, 2017 at 5:43 PM Lang Hames wrote:
>>
>>> Hi Zachary,
>>>
>>> ... Then instead of Expected you could have WithDiagnostics that
enforces the proper semantics.
>>>
>>>
>>> You mean something like an ErrorAnd? Chris Bieneman floated that idea
>>> for some libObject code but we haven't got around to implementing it. If it
>>> were generically useful we could do something like that.
>>>
>>> Cheers,
>>> Lang.
>>>
>>>
>>> On Mon, May 1, 2017 at 5:36 PM, Zachary Turner
>>> wrote:
>>>
Is there any chance of introducing something like make_status() into
llvm/Error.h, that constructs the llvm::Error in such a way that it still
interoperates nicely with everything else? Then instead of Expected you
could have WithDiagnostics that enforces the proper semantics.
On Mon, May 1, 2017 at 5:33 PM Zachary Turner
wrote:
> On Mon, May 1, 2017 at 5:27 PM Jim Ingham wrote:
>
>>
>> > On May 1, 2017, at 4:52 PM, Zachary Turner
>> wrote:
>> >
>> > Yea, grouping the error and the result together is one of the most
>> compelling features of it. It's called Expected, so where we would
>> currently write something like:
>> >
>> > int getNumberOfSymbols(Error &Err) {}
>> >
>> > or
>> >
>> > Error getNumberOfSymbols(int &Count) {}
>> >
>> > You would now write:
>> >
>> > Expected getNumberOfSymbols() {
>> >if (foo) return 1;
>> >else return make_error("No symbols!");
>> > }
>> >
>> > and on the caller side you write:
>> >
>> > Error processAllSymbols() {
>> > if (auto Syms = getNumberOfSymbols()) {
>> > outs() << "There are " << *Syms << " symbols!";
>> > } else {
>> > return Syms.takeError();
>> > // alternatively, you could write:
>> > // consumeError(Syms.takeError());
>> > // return Error::success();
>> > }
>> > }
>> >
>>
>> Interesting.
>>
>> This pattern doesn't quite work for fetching symbols - maybe that
>> really is more suitable as a Status than an Error. After all, number of
>> symbols == 0 is not necessarily an error, there just might not have been
>> any symbols (e.g. a fully stripped main); and I'm going to work on
>> whatever
>> symbols I get back, since there's nothing I can do about the ones that
>> didn't make it. I just want to propagate the error so the user knows
>> that
>> there was a problem.
>>
>> Jim
>>
>
> Sure, that was just a made up example. You could imagine that being
> some private function deep in the implementation details of a symbol
> parser, where you've got some header that's supposed to be N bytes, and
> getNumberOfSymbols() seeks to offset 42 and reads a 4 byte value and
> returns it, but the function sees that there's only 40 bytes in the
> header,
> so it's not that there's no symbols, it's that something is seriously
> messed up.
>
> In that case you could return an error such as this.
>
> Of course, the person who called this function can either propagate
> it, deal with it some other way and mask it, or whatever. Mostly I was
> just trying to show what the syntax looked like for grouping return values
> with errors.
>
>>>
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
Re: [lldb-dev] Renaming lldb_private::Error
Hi Zachary,
... Then instead of Expected you could have WithDiagnostics that
> enforces the proper semantics.
You mean something like an ErrorAnd? Chris Bieneman floated that idea
for some libObject code but we haven't got around to implementing it. If it
were generically useful we could do something like that.
Cheers,
Lang.
On Mon, May 1, 2017 at 5:36 PM, Zachary Turner wrote:
> Is there any chance of introducing something like make_status() into
> llvm/Error.h, that constructs the llvm::Error in such a way that it still
> interoperates nicely with everything else? Then instead of Expected you
> could have WithDiagnostics that enforces the proper semantics.
>
> On Mon, May 1, 2017 at 5:33 PM Zachary Turner wrote:
>
>> On Mon, May 1, 2017 at 5:27 PM Jim Ingham wrote:
>>
>>>
>>> > On May 1, 2017, at 4:52 PM, Zachary Turner wrote:
>>> >
>>> > Yea, grouping the error and the result together is one of the most
>>> compelling features of it. It's called Expected, so where we would
>>> currently write something like:
>>> >
>>> > int getNumberOfSymbols(Error &Err) {}
>>> >
>>> > or
>>> >
>>> > Error getNumberOfSymbols(int &Count) {}
>>> >
>>> > You would now write:
>>> >
>>> > Expected getNumberOfSymbols() {
>>> >if (foo) return 1;
>>> >else return make_error("No symbols!");
>>> > }
>>> >
>>> > and on the caller side you write:
>>> >
>>> > Error processAllSymbols() {
>>> > if (auto Syms = getNumberOfSymbols()) {
>>> > outs() << "There are " << *Syms << " symbols!";
>>> > } else {
>>> > return Syms.takeError();
>>> > // alternatively, you could write:
>>> > // consumeError(Syms.takeError());
>>> > // return Error::success();
>>> > }
>>> > }
>>> >
>>>
>>> Interesting.
>>>
>>> This pattern doesn't quite work for fetching symbols - maybe that really
>>> is more suitable as a Status than an Error. After all, number of symbols
>>> == 0 is not necessarily an error, there just might not have been any
>>> symbols (e.g. a fully stripped main); and I'm going to work on whatever
>>> symbols I get back, since there's nothing I can do about the ones that
>>> didn't make it. I just want to propagate the error so the user knows that
>>> there was a problem.
>>>
>>> Jim
>>>
>>
>> Sure, that was just a made up example. You could imagine that being some
>> private function deep in the implementation details of a symbol parser,
>> where you've got some header that's supposed to be N bytes, and
>> getNumberOfSymbols() seeks to offset 42 and reads a 4 byte value and
>> returns it, but the function sees that there's only 40 bytes in the header,
>> so it's not that there's no symbols, it's that something is seriously
>> messed up.
>>
>> In that case you could return an error such as this.
>>
>> Of course, the person who called this function can either propagate it,
>> deal with it some other way and mask it, or whatever. Mostly I was just
>> trying to show what the syntax looked like for grouping return values with
>> errors.
>>
>
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
Re: [lldb-dev] Renaming lldb_private::Error
Yeah - operations that may produce an valid result *and* a diagnostic would
be better suited to Status, or a pair where we want a hard
requirement that the API client check the diagnostic.
- Lang.
On Mon, May 1, 2017 at 5:27 PM, Jim Ingham wrote:
>
> > On May 1, 2017, at 4:52 PM, Zachary Turner wrote:
> >
> > Yea, grouping the error and the result together is one of the most
> compelling features of it. It's called Expected, so where we would
> currently write something like:
> >
> > int getNumberOfSymbols(Error &Err) {}
> >
> > or
> >
> > Error getNumberOfSymbols(int &Count) {}
> >
> > You would now write:
> >
> > Expected getNumberOfSymbols() {
> >if (foo) return 1;
> >else return make_error("No symbols!");
> > }
> >
> > and on the caller side you write:
> >
> > Error processAllSymbols() {
> > if (auto Syms = getNumberOfSymbols()) {
> > outs() << "There are " << *Syms << " symbols!";
> > } else {
> > return Syms.takeError();
> > // alternatively, you could write:
> > // consumeError(Syms.takeError());
> > // return Error::success();
> > }
> > }
> >
>
> Interesting.
>
> This pattern doesn't quite work for fetching symbols - maybe that really
> is more suitable as a Status than an Error. After all, number of symbols
> == 0 is not necessarily an error, there just might not have been any
> symbols (e.g. a fully stripped main); and I'm going to work on whatever
> symbols I get back, since there's nothing I can do about the ones that
> didn't make it. I just want to propagate the error so the user knows that
> there was a problem.
>
> Jim
>
>
> >
> > On Mon, May 1, 2017 at 4:47 PM Jim Ingham wrote:
> >
> > > On May 1, 2017, at 3:29 PM, Zachary Turner wrote:
> > >
> > > I'm confused. By having the library assert, you are informed of
> places where you didn't do a good job of backing from errors, thereby
> allowing you to do a *better* job.
> > >
> > > You said this earlier:
> > >
> > > > But a larger point about asserting as a result of errors is that it
> makes it seem to the lldb developer like once you've raised an assert on
> error your job is done. You've stopped the error from propagating, two
> points!
> > >
> > > But when you're using llvm::Error, no developer is actively thinking
> about asserts. Nobody is thinking "well the library is going to assert, so
> my job is done here " because that doesn't make any sense. It's going
> to assert even if the operation was successful
> > >
> > > Your job can't possibly be done because if you don't check the error,
> you will assert 100% of the time you execute that codepath. You might as
> well have just written int x = *nullptr; Surely nobody could agree that
> their job is done after writing int x = *nullptr; in their code.
> > >
> > > If you write this:
> > >
> > > Error foo(int &x) {
> > > x = 42;
> > > return Error::success();
> > > }
> > >
> > > void bar() {
> > > int x;
> > > foo(x);
> > > cout << x;
> > > }
> > >
> > > Then this code is going to assert. It doesn't matter that no error
> actually occurred. That is why I'm saying it is strictly a win, no matter
> what, in all situations. If you forget to check an error code, you
> *necessarily* aren't doing the best possible job backing out of the code in
> case an error does occur. Now you will find it and be able to fix it.
> >
> > Yeah, Lang was just explaining this. I think I was over-reacting to the
> asserts part because llvm's aggressive use of early failure was a real
> problem for lldb. So my hackles go up when something like it comes up
> again.
> >
> > In practical terms, lldb quite often uses another measure than the error
> to decide how it's going to proceed. I ask for some symbols, and I get
> some, but at the same time, one of 10 object files had some bad DWARF so an
> error was produced. I'll pass that error along for informational purposes,
> but I don't really care, I'm still going to set breakpoints on all the
> symbols I found. Lang said it is possible to gang something like the
> "number of symbols" and the error, so that checking the number of symbols
> automatically ticks the error box as well. If eventually ever comes we'll
> have to deal with this sort of complication.
> >
> > As for Error -> Status to avoid confusion, that seems fine, though if
> you are going to do it, I agree with Pavel it would be gross to have
> "Status error;" all over the place.
> >
> > Jim
> >
> >
> > >
> > > On Mon, May 1, 2017 at 3:19 PM Jim Ingham wrote:
> > >
> > > > On May 1, 2017, at 3:12 PM, Zachary Turner
> wrote:
> > > >
> > > > Does Xcode ship with a build of LLDB that has asserts enabled?
> Because if not it shouldn't affect anything. If so, can you shed some
> light on why?
> > >
> > > Not sure that's entirely relevant. The point is not to make failure
> points assert then turn them off in production because they shouldn't
> assert. The point is to make sure that instead of asserting you always do
> the best job y
Re: [lldb-dev] Renaming lldb_private::Error
For instances where we know that we just want to log errors and continue
with some sensible default, we could introduce a new utility template along
the lines of:
template
T logUnwrap(Expected Result, T DefaultValue = T()) {
if (Result)
return std::move(*Result);
else {
log(Result.takeError());
return DefaultValue;
}
}
Now we can write:
T Result = logUnwrap(foo(...));
and have a strong guarantee that errors will be logged, and that we know
the value of Result if an error occurs.
Cheers,
Lang.
On Mon, May 1, 2017 at 4:52 PM, Zachary Turner wrote:
> Yea, grouping the error and the result together is one of the most
> compelling features of it. It's called Expected, so where we would
> currently write something like:
>
> int getNumberOfSymbols(Error &Err) {}
>
> or
>
> Error getNumberOfSymbols(int &Count) {}
>
> You would now write:
>
> Expected getNumberOfSymbols() {
>if (foo) return 1;
>else return make_error("No symbols!");
> }
>
> and on the caller side you write:
>
> Error processAllSymbols() {
> if (auto Syms = getNumberOfSymbols()) {
> outs() << "There are " << *Syms << " symbols!";
> } else {
> return Syms.takeError();
> // alternatively, you could write:
> // consumeError(Syms.takeError());
> // return Error::success();
> }
> }
>
>
> On Mon, May 1, 2017 at 4:47 PM Jim Ingham wrote:
>
>>
>> > On May 1, 2017, at 3:29 PM, Zachary Turner wrote:
>> >
>> > I'm confused. By having the library assert, you are informed of places
>> where you didn't do a good job of backing from errors, thereby allowing you
>> to do a *better* job.
>> >
>> > You said this earlier:
>> >
>> > > But a larger point about asserting as a result of errors is that it
>> makes it seem to the lldb developer like once you've raised an assert on
>> error your job is done. You've stopped the error from propagating, two
>> points!
>> >
>> > But when you're using llvm::Error, no developer is actively thinking
>> about asserts. Nobody is thinking "well the library is going to assert, so
>> my job is done here " because that doesn't make any sense. It's going
>> to assert even if the operation was successful
>> >
>> > Your job can't possibly be done because if you don't check the error,
>> you will assert 100% of the time you execute that codepath. You might as
>> well have just written int x = *nullptr; Surely nobody could agree that
>> their job is done after writing int x = *nullptr; in their code.
>> >
>> > If you write this:
>> >
>> > Error foo(int &x) {
>> > x = 42;
>> > return Error::success();
>> > }
>> >
>> > void bar() {
>> > int x;
>> > foo(x);
>> > cout << x;
>> > }
>> >
>> > Then this code is going to assert. It doesn't matter that no error
>> actually occurred. That is why I'm saying it is strictly a win, no matter
>> what, in all situations. If you forget to check an error code, you
>> *necessarily* aren't doing the best possible job backing out of the code in
>> case an error does occur. Now you will find it and be able to fix it.
>>
>> Yeah, Lang was just explaining this. I think I was over-reacting to the
>> asserts part because llvm's aggressive use of early failure was a real
>> problem for lldb. So my hackles go up when something like it comes up
>> again.
>>
>> In practical terms, lldb quite often uses another measure than the error
>> to decide how it's going to proceed. I ask for some symbols, and I get
>> some, but at the same time, one of 10 object files had some bad DWARF so an
>> error was produced. I'll pass that error along for informational purposes,
>> but I don't really care, I'm still going to set breakpoints on all the
>> symbols I found. Lang said it is possible to gang something like the
>> "number of symbols" and the error, so that checking the number of symbols
>> automatically ticks the error box as well. If eventually ever comes we'll
>> have to deal with this sort of complication.
>>
>> As for Error -> Status to avoid confusion, that seems fine, though if you
>> are going to do it, I agree with Pavel it would be gross to have "Status
>> error;" all over the place.
>>
>> Jim
>>
>>
>> >
>> > On Mon, May 1, 2017 at 3:19 PM Jim Ingham wrote:
>> >
>> > > On May 1, 2017, at 3:12 PM, Zachary Turner
>> wrote:
>> > >
>> > > Does Xcode ship with a build of LLDB that has asserts enabled?
>> Because if not it shouldn't affect anything. If so, can you shed some
>> light on why?
>> >
>> > Not sure that's entirely relevant. The point is not to make failure
>> points assert then turn them off in production because they shouldn't
>> assert. The point is to make sure that instead of asserting you always do
>> the best job you can backing out from any error.
>> >
>> > Jim
>> >
>> >
>> > >
>> > > On Mon, May 1, 2017 at 3:08 PM Jim Ingham wrote:
>> > >
>> > > > On May 1, 2017, at 2:51 PM, Zachary Turner
>> wrote:
>> > > >
>> > > > I think we agree about the SB layer. You can't have mandatory
>> checking on things that
Re: [lldb-dev] Renaming lldb_private::Error
> > /// ...In the future we may wish to switch to a > > > /// registration mechanism where new error types can be registered at > > > /// runtime instead of a hard coded scheme. I immediately regret my decision to copy-paste from terminal. :/ - Lang. On Sun, Apr 30, 2017 at 10:39 AM, Lang Hames wrote: > Hi Zachary, > > I'm new to LLDB so take my opinion with a grain of salt, but this sounds > like a good idea to me. LLDB is likely to encounter more and more LLVM APIs > using llvm::Error in the future, so renaming lldb_private::Error to reduce > confusion seems sensible. > > Replacing lldb_private::Error at some point in the future probably makes > sense too. The author of lldb_private::Error seems to have had a similar > idea: > > /// ...In the future we may wish to switch to a > > > /// registration mechanism where new error types can be registered at > > > /// runtime instead of a hard coded scheme. > > The challenge here will be interoperability with the python APIs, which > look like they map the current lldb_private::Error into Python. That will > take some thought, but I think it should be possible. > > For any LLDB devs who are interested in llvm::Error, the lightning talk > that introduced it is at: https://www.youtube.com/watch?v=Wq8fNK98WGw , > and the API is covered in more detail in the LLVM programmer's manual: > http://llvm.org/docs/ProgrammersManual.html#recoverable-errors . > > Cheers, > Lang. > > On Fri, Apr 28, 2017 at 8:40 PM, Zachary Turner > wrote: > >> I have a patch locally that renames lldb_private::Error to >> lldb_private::LLDBError. As you might expect, this is a pretty large patch >> so I don't plan to put it up for review, but since it's kind of a >> fundamental class I figured I should at least start a discussion. >> >> The primary motivation for this is to enable cleaner interop between >> lldb's Error class and llvm::Error. Currently there is an ambiguity >> between the two. Obviously they are scoped in different namespaces, but >> it's still confusing when looking at code to see such similarly named >> classes. >> >> There are a number of advantages to llvm::Error over lldb Error which I'm >> happy to elaborate on if anyone needs some clarification (I've also cc'ed >> lang on here, who wrote the original llvm Error implementation). >> >> Long term I would eventually like to deprecate lldb's Error class >> entirely, and switch entirely to llvm::Error. An intermediate transition >> phase would likely involve making something like LLDBWrapError which >> interoperates with llvm::Error and wraps an lldb_private::LLDBError. >> >> For now though, I'm only proposing a very simple rename with minimal >> invasiveness. >> >> Comments? >> > > ___ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
Re: [lldb-dev] Renaming lldb_private::Error
Hi Zachary, I'm new to LLDB so take my opinion with a grain of salt, but this sounds like a good idea to me. LLDB is likely to encounter more and more LLVM APIs using llvm::Error in the future, so renaming lldb_private::Error to reduce confusion seems sensible. Replacing lldb_private::Error at some point in the future probably makes sense too. The author of lldb_private::Error seems to have had a similar idea: /// ...In the future we may wish to switch to a /// registration mechanism where new error types can be registered at /// runtime instead of a hard coded scheme. The challenge here will be interoperability with the python APIs, which look like they map the current lldb_private::Error into Python. That will take some thought, but I think it should be possible. For any LLDB devs who are interested in llvm::Error, the lightning talk that introduced it is at: https://www.youtube.com/watch?v=Wq8fNK98WGw , and the API is covered in more detail in the LLVM programmer's manual: http://llvm.org/docs/ProgrammersManual.html#recoverable-errors . Cheers, Lang. On Fri, Apr 28, 2017 at 8:40 PM, Zachary Turner wrote: > I have a patch locally that renames lldb_private::Error to > lldb_private::LLDBError. As you might expect, this is a pretty large patch > so I don't plan to put it up for review, but since it's kind of a > fundamental class I figured I should at least start a discussion. > > The primary motivation for this is to enable cleaner interop between > lldb's Error class and llvm::Error. Currently there is an ambiguity > between the two. Obviously they are scoped in different namespaces, but > it's still confusing when looking at code to see such similarly named > classes. > > There are a number of advantages to llvm::Error over lldb Error which I'm > happy to elaborate on if anyone needs some clarification (I've also cc'ed > lang on here, who wrote the original llvm Error implementation). > > Long term I would eventually like to deprecate lldb's Error class > entirely, and switch entirely to llvm::Error. An intermediate transition > phase would likely involve making something like LLDBWrapError which > interoperates with llvm::Error and wraps an lldb_private::LLDBError. > > For now though, I'm only proposing a very simple rename with minimal > invasiveness. > > Comments? > ___ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
