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

[Greg Clayton via lldb-commits]

Thanks for fixing this is the right way and taking the time!

Greg


> On Mar 20, 2018, at 12:49 PM, Davide Italiano via lldb-commits 
>  wrote:
> 
> Fixed in a nicer/cleaner way (that doesn't regress the current
> behavior), thank you everybody for your excellent feedback!
> 
> davide@Davidinos-Mac-Pro ~/w/l/llvm-project-20170507> git llvm push
> Pushing 1 commit:
>  8875fcce772 [ExpressionParser] Re-implement r327356 in a less disruptive way.
> Sendinglldb/trunk/include/lldb/Symbol/ClangASTContext.h
> Deleting   lldb/trunk/lit/Expr/Inputs/basic.cpp
> Deleting   lldb/trunk/lit/Expr/TestCallCppSym.test
> Adding 
> lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/operator-overload
> Adding 
> lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/operator-overload/Makefile
> Adding 
> lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/operator-overload/TestOperatorOverload.py
> Adding 
> lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/operator-overload/a.cpp
> Adding 
> lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/operator-overload/b.cpp
> Sending
> lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
> Sending
> lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
> Sendinglldb/trunk/source/Symbol/ClangASTContext.cpp
> Transmitting file data done
> Committing transaction...
> Committed revision 328025.
> Committed 8875fcce772 to svn.
> 
> Thanks,
> 
> --
> Davide
> 
> On Thu, Mar 15, 2018 at 8:36 AM, Davide Italiano  
> wrote:
>> On Wed, Mar 14, 2018 at 1:52 AM, Pavel Labath  wrote:
>>> I'm not familiar with all of the magic we do when we synthesize clang Decls,
>>> but I feel I should point out that we can't get out of business of
>>> sanity-checking the declarations we inject into clang. The reason for that
>>> is, even if we had debug info for operator==, the debug info itself could
>>> describe it's prototype as operator==(...) (due to a compiler bug, corrupt
>>> file, or whatever). So we still need to make sure that the declarations we
>>> synthesize from debug info don't violate clang's invariants (and that's what
>>> we try to do at present, cf.
>>> ClangASTContext::CheckOverloadedOperatorParameterCount).
>>> 
>>> So maybe the solution here is not to refuse injecting any declarations
>>> without debug info, but instead to make sure that whatever declarations we
>>> inject that way satisfy the same validity criteria as the ones we synthesize
>>> from the debug info?
>>> 
>> 
>> I'll think about this more. On a more practical note, I was a able to
>> reproduce this with a fairly self contained C++ program :)
>> 
>> dcci@Davides-MacBook-Pro ~/w/l/b/bin> cat patatino.cpp
>> class Patatino {
>> public:
>>  double _blah;
>>  Patatino(int blah) : _blah(blah) {}
>> };
>> 
>> bool operator==(const Patatino& a, const Patatino& b) {
>>  return a._blah < b._blah;
>> }
>> 
>> 
>> dcci@Davides-MacBook-Pro ~/w/l/b/bin> cat patatuccio.cpp
>> class Baciotto {
>> public:
>>  int _meh;
>>  Baciotto(int meh) : _meh(meh) {}
>> };
>> 
>> int main(void) {
>>  Baciotto x(12);
>>  return 0;
>> }
>> 
>> 
>> $ ./clang++ patatuccio.cpp -o patatuccio.o -c -g
>> $ ./clang++ patatino.cpp -o patatino.o -c
>> $ ./clang++ patatino.o patatuccio.o -o patatuccio
>> 
>> $ nm ./patatuccio
>> 00010f70 t __ZN8BaciottoC1Ei
>> 00010fa0 t __ZN8BaciottoC2Ei.
>> 00010f10 T __ZeqRK8PatatinoS1_.  <--- this is the wrong symbol 
>> picked up
>> 0001 T __mh_execute_header
>> 00010f40 T _main
>> U dyld_stub_binder
>> 
>> $ echo '__ZeqRK8PatatinoS1_' | c++filt
>> operator==(Patatino const&, Patatino const&)
>> 
>> And in lldb:
>> 
>> (lldb) n
>> Process 35027 stopped
>> * thread #1, queue = 'com.apple.main-thread', stop reason = step over
>>frame #0: 0x00010f5f patatuccio`main at patatuccio.cpp:9
>>   6
>>   7   int main(void) {
>>   8Baciotto x(12);
>> -> 9return 0;
>>   10   }
>> (lldb) expr x == nil
>> Assertion failed: (i < getNumParams() && "Illegal param #"), function
>> getParamDecl, file
>> /Users/dcci/work/llvm/llvm/tools/clang/include/clang/AST/Decl.h, line
>> 2232.
>> fish: './lldb' terminated by signal SIGABRT (Abort)
>> 
>> 
>> I'll try debugging this more.
>> 
>> Thanks!
>> 
>> --
>> Davide
> ___
> 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] [lldb] r327356 - [ExpressionParser] Fix crash when evaluating invalid expresssions.

[Davide Italiano via lldb-commits]

Fixed in a nicer/cleaner way (that doesn't regress the current
behavior), thank you everybody for your excellent feedback!

davide@Davidinos-Mac-Pro ~/w/l/llvm-project-20170507> git llvm push
Pushing 1 commit:
  8875fcce772 [ExpressionParser] Re-implement r327356 in a less disruptive way.
Sendinglldb/trunk/include/lldb/Symbol/ClangASTContext.h
Deleting   lldb/trunk/lit/Expr/Inputs/basic.cpp
Deleting   lldb/trunk/lit/Expr/TestCallCppSym.test
Adding 
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/operator-overload
Adding 
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/operator-overload/Makefile
Adding 
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/operator-overload/TestOperatorOverload.py
Adding 
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/operator-overload/a.cpp
Adding 
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/operator-overload/b.cpp
Sending
lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
Sending
lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
Sendinglldb/trunk/source/Symbol/ClangASTContext.cpp
Transmitting file data done
Committing transaction...
Committed revision 328025.
Committed 8875fcce772 to svn.

Thanks,

--
Davide

On Thu, Mar 15, 2018 at 8:36 AM, Davide Italiano  wrote:
> On Wed, Mar 14, 2018 at 1:52 AM, Pavel Labath  wrote:
>> I'm not familiar with all of the magic we do when we synthesize clang Decls,
>> but I feel I should point out that we can't get out of business of
>> sanity-checking the declarations we inject into clang. The reason for that
>> is, even if we had debug info for operator==, the debug info itself could
>> describe it's prototype as operator==(...) (due to a compiler bug, corrupt
>> file, or whatever). So we still need to make sure that the declarations we
>> synthesize from debug info don't violate clang's invariants (and that's what
>> we try to do at present, cf.
>> ClangASTContext::CheckOverloadedOperatorParameterCount).
>>
>> So maybe the solution here is not to refuse injecting any declarations
>> without debug info, but instead to make sure that whatever declarations we
>> inject that way satisfy the same validity criteria as the ones we synthesize
>> from the debug info?
>>
>
> I'll think about this more. On a more practical note, I was a able to
> reproduce this with a fairly self contained C++ program :)
>
> dcci@Davides-MacBook-Pro ~/w/l/b/bin> cat patatino.cpp
> class Patatino {
> public:
>   double _blah;
>   Patatino(int blah) : _blah(blah) {}
> };
>
> bool operator==(const Patatino& a, const Patatino& b) {
>   return a._blah < b._blah;
> }
>
>
> dcci@Davides-MacBook-Pro ~/w/l/b/bin> cat patatuccio.cpp
> class Baciotto {
> public:
>   int _meh;
>   Baciotto(int meh) : _meh(meh) {}
> };
>
> int main(void) {
>   Baciotto x(12);
>   return 0;
> }
>
>
> $ ./clang++ patatuccio.cpp -o patatuccio.o -c -g
> $ ./clang++ patatino.cpp -o patatino.o -c
> $ ./clang++ patatino.o patatuccio.o -o patatuccio
>
> $ nm ./patatuccio
> 00010f70 t __ZN8BaciottoC1Ei
> 00010fa0 t __ZN8BaciottoC2Ei.
> 00010f10 T __ZeqRK8PatatinoS1_.  <--- this is the wrong symbol picked 
> up
> 0001 T __mh_execute_header
> 00010f40 T _main
>  U dyld_stub_binder
>
> $ echo '__ZeqRK8PatatinoS1_' | c++filt
> operator==(Patatino const&, Patatino const&)
>
> And in lldb:
>
> (lldb) n
> Process 35027 stopped
> * thread #1, queue = 'com.apple.main-thread', stop reason = step over
> frame #0: 0x00010f5f patatuccio`main at patatuccio.cpp:9
>6
>7   int main(void) {
>8Baciotto x(12);
> -> 9return 0;
>10   }
> (lldb) expr x == nil
> Assertion failed: (i < getNumParams() && "Illegal param #"), function
> getParamDecl, file
> /Users/dcci/work/llvm/llvm/tools/clang/include/clang/AST/Decl.h, line
> 2232.
> fish: './lldb' terminated by signal SIGABRT (Abort)
>
>
> I'll try debugging this more.
>
> Thanks!
>
> --
> Davide
___
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.

[Davide Italiano via lldb-commits]

On Wed, Mar 14, 2018 at 1:52 AM, Pavel Labath  wrote:
> I'm not familiar with all of the magic we do when we synthesize clang Decls,
> but I feel I should point out that we can't get out of business of
> sanity-checking the declarations we inject into clang. The reason for that
> is, even if we had debug info for operator==, the debug info itself could
> describe it's prototype as operator==(...) (due to a compiler bug, corrupt
> file, or whatever). So we still need to make sure that the declarations we
> synthesize from debug info don't violate clang's invariants (and that's what
> we try to do at present, cf.
> ClangASTContext::CheckOverloadedOperatorParameterCount).
>
> So maybe the solution here is not to refuse injecting any declarations
> without debug info, but instead to make sure that whatever declarations we
> inject that way satisfy the same validity criteria as the ones we synthesize
> from the debug info?
>

I'll think about this more. On a more practical note, I was a able to
reproduce this with a fairly self contained C++ program :)

dcci@Davides-MacBook-Pro ~/w/l/b/bin> cat patatino.cpp
class Patatino {
public:
  double _blah;
  Patatino(int blah) : _blah(blah) {}
};

bool operator==(const Patatino& a, const Patatino& b) {
  return a._blah < b._blah;
}


dcci@Davides-MacBook-Pro ~/w/l/b/bin> cat patatuccio.cpp
class Baciotto {
public:
  int _meh;
  Baciotto(int meh) : _meh(meh) {}
};

int main(void) {
  Baciotto x(12);
  return 0;
}


$ ./clang++ patatuccio.cpp -o patatuccio.o -c -g
$ ./clang++ patatino.cpp -o patatino.o -c
$ ./clang++ patatino.o patatuccio.o -o patatuccio

$ nm ./patatuccio
00010f70 t __ZN8BaciottoC1Ei
00010fa0 t __ZN8BaciottoC2Ei.
00010f10 T __ZeqRK8PatatinoS1_.  <--- this is the wrong symbol picked up
0001 T __mh_execute_header
00010f40 T _main
 U dyld_stub_binder

$ echo '__ZeqRK8PatatinoS1_' | c++filt
operator==(Patatino const&, Patatino const&)

And in lldb:

(lldb) n
Process 35027 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = step over
frame #0: 0x00010f5f patatuccio`main at patatuccio.cpp:9
   6
   7   int main(void) {
   8Baciotto x(12);
-> 9return 0;
   10   }
(lldb) expr x == nil
Assertion failed: (i < getNumParams() && "Illegal param #"), function
getParamDecl, file
/Users/dcci/work/llvm/llvm/tools/clang/include/clang/AST/Decl.h, line
2232.
fish: './lldb' terminated by signal SIGABRT (Abort)


I'll try debugging this more.

Thanks!

--
Davide
___
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.

[Pavel Labath via lldb-commits]

I'm not familiar with all of the magic we do when we synthesize clang
Decls, but I feel I should point out that we can't get out of business of
sanity-checking the declarations we inject into clang. The reason for that
is, even if we had debug info for operator==, the debug info itself could
describe it's prototype as operator==(...) (due to a compiler bug, corrupt
file, or whatever). So we still need to make sure that the declarations we
synthesize from debug info don't violate clang's invariants (and that's
what we try to do at present, cf.
ClangASTContext::CheckOverloadedOperatorParameterCount).

So maybe the solution here is not to refuse injecting any declarations
without debug info, but instead to make sure that whatever declarations we
inject that way satisfy the same validity criteria as the ones we
synthesize from the debug info?

cheers,
pl

On Wed, 14 Mar 2018 at 01:25, Davide Italiano via lldb-commits <
lldb-commits@lists.llvm.org> wrote:

> On Tue, Mar 13, 2018 at 2:43 PM, Jason Molenda  wrote:
> >
> >
> >> On Mar 13, 2018, at 11:51 AM, Davide Italiano via lldb-commits <
> lldb-commits@lists.llvm.org> wrote:
> >>
> >> We had the report of a crash where lldb was setting a conditional
> >> breakpoint on an invalid condition (CGRect == pointer, which is,
> >> ill-formed).
> >> The symbol-based resolution of lldb was picking a random operator==,
> >> inserting a generic function decl operator== in the context because it
> >> happened to find a matching symbol somewhere in the system.
> >>
> >> clang expects operator== to have at least one argument, so you end up
> >> hitting this assertion in Sema.
> >>
> >> (lldb) Assertion failed: (i < getNumParams() && "Illegal param #"),
> >> function getParamDecl, file
> >>
> /Users/davide/work/llvm-monorepo/llvm-project-20170507/llvm/tools/clang/include/clang/AST/Decl.h,
> >> line 2232.
> >>
> >> So, to answer your question, Greg, I just want lldb to not inject
> >> arbitrary C++ func decl. What do you think is the best way of
> >> achieving this?
> >>
> >
> >
> > I put together a repo case that might help make this clearer (attached)
> >
> >
> >
> >
> >
> > we have a test program with three translation units.  One has C++
> methods and was built with debug info ("foo"), one has C++ methods and was
> built without debug info ("bar").  It tries calling these from lldb:
> >
> >
> > (lldb) p (void)foo('c')
> > in foo(char)
> > (lldb) p (void)foo(5)
> > in foo(int)
> > (lldb) p (void)foo(5, 5)
> > in foo(int, int)
> >
> > We had debug info for foo(), called the correct methods.
> >
> >
> > (lldb) p (void)bar('c')
> > in bar(char *)
> > (lldb) p (void)bar(5)
> > in bar(char *)
> > (lldb) p (void)bar(5, 5)
> > in bar(char *)
> >
> >
> > Here, we have no debug info for bar(), so we grabbed the first bar()
> method we found and used it for all calls.  This is a problem.
> >
> >
> > (lldb) p (void)_Z3barc('c')
> > in bar(char)
> > (lldb) p (void)_Z3bari(5)
> > in bar(int)
> > (lldb) p (void)_Z3barii(5,5)
> > in bar(int, int)
> >
> > Calling the mangled name bar()'s directly works as expected.
> >
> >
> >
> > Davide is trying to solve that middle one.  I think the problem he was
> working on is where we had an expression using operator== and the first
> "decl" we found of this was in a no-debug-info translation unit, and that
> randomly chosen operator== was used when there WAS debug info for this
> operator== in a different translation unit in the process.
> >
> > The question I have is -- should this even be allowed.  Should you be
> able to call a C++ method using a demangled name with no debug info?  I'm
> trying to think of a case where people do this today.  Clearly it does not
> work correctly today for overloaded functions.  And apparently this could
> be chosen over a debug info definition that might appear elsewhere in the
> process?  I didn't try to test that.
> >
>
> The debug info version, if present has precedence. The scary bit is
> that if you have several symbols matching the decl name (e.g.
> `operator==`) lldb will pick a random one [the last one in the list it
> builds according to some order]. I don't think this is the expected
> behavior, but this is what we have today.
>
> Thanks,
>
> --
> Davide
> ___
> 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] [lldb] r327356 - [ExpressionParser] Fix crash when evaluating invalid expresssions.

[Davide Italiano via lldb-commits]

On Tue, Mar 13, 2018 at 2:43 PM, Jason Molenda  wrote:
>
>
>> On Mar 13, 2018, at 11:51 AM, Davide Italiano via lldb-commits 
>>  wrote:
>>
>> We had the report of a crash where lldb was setting a conditional
>> breakpoint on an invalid condition (CGRect == pointer, which is,
>> ill-formed).
>> The symbol-based resolution of lldb was picking a random operator==,
>> inserting a generic function decl operator== in the context because it
>> happened to find a matching symbol somewhere in the system.
>>
>> clang expects operator== to have at least one argument, so you end up
>> hitting this assertion in Sema.
>>
>> (lldb) Assertion failed: (i < getNumParams() && "Illegal param #"),
>> function getParamDecl, file
>> /Users/davide/work/llvm-monorepo/llvm-project-20170507/llvm/tools/clang/include/clang/AST/Decl.h,
>> line 2232.
>>
>> So, to answer your question, Greg, I just want lldb to not inject
>> arbitrary C++ func decl. What do you think is the best way of
>> achieving this?
>>
>
>
> I put together a repo case that might help make this clearer (attached)
>
>
>
>
>
> we have a test program with three translation units.  One has C++ methods and 
> was built with debug info ("foo"), one has C++ methods and was built without 
> debug info ("bar").  It tries calling these from lldb:
>
>
> (lldb) p (void)foo('c')
> in foo(char)
> (lldb) p (void)foo(5)
> in foo(int)
> (lldb) p (void)foo(5, 5)
> in foo(int, int)
>
> We had debug info for foo(), called the correct methods.
>
>
> (lldb) p (void)bar('c')
> in bar(char *)
> (lldb) p (void)bar(5)
> in bar(char *)
> (lldb) p (void)bar(5, 5)
> in bar(char *)
>
>
> Here, we have no debug info for bar(), so we grabbed the first bar() method 
> we found and used it for all calls.  This is a problem.
>
>
> (lldb) p (void)_Z3barc('c')
> in bar(char)
> (lldb) p (void)_Z3bari(5)
> in bar(int)
> (lldb) p (void)_Z3barii(5,5)
> in bar(int, int)
>
> Calling the mangled name bar()'s directly works as expected.
>
>
>
> Davide is trying to solve that middle one.  I think the problem he was 
> working on is where we had an expression using operator== and the first 
> "decl" we found of this was in a no-debug-info translation unit, and that 
> randomly chosen operator== was used when there WAS debug info for this 
> operator== in a different translation unit in the process.
>
> The question I have is -- should this even be allowed.  Should you be able to 
> call a C++ method using a demangled name with no debug info?  I'm trying to 
> think of a case where people do this today.  Clearly it does not work 
> correctly today for overloaded functions.  And apparently this could be 
> chosen over a debug info definition that might appear elsewhere in the 
> process?  I didn't try to test that.
>

The debug info version, if present has precedence. The scary bit is
that if you have several symbols matching the decl name (e.g.
`operator==`) lldb will pick a random one [the last one in the list it
builds according to some order]. I don't think this is the expected
behavior, but this is what we have today.

Thanks,

--
Davide
___
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.

[Jason Molenda via lldb-commits]

> On Mar 13, 2018, at 11:51 AM, Davide Italiano via lldb-commits 
>  wrote:
> 
> We had the report of a crash where lldb was setting a conditional
> breakpoint on an invalid condition (CGRect == pointer, which is,
> ill-formed).
> The symbol-based resolution of lldb was picking a random operator==,
> inserting a generic function decl operator== in the context because it
> happened to find a matching symbol somewhere in the system.
> 
> clang expects operator== to have at least one argument, so you end up
> hitting this assertion in Sema.
> 
> (lldb) Assertion failed: (i < getNumParams() && "Illegal param #"),
> function getParamDecl, file
> /Users/davide/work/llvm-monorepo/llvm-project-20170507/llvm/tools/clang/include/clang/AST/Decl.h,
> line 2232.
> 
> So, to answer your question, Greg, I just want lldb to not inject
> arbitrary C++ func decl. What do you think is the best way of
> achieving this?
> 


I put together a repo case that might help make this clearer (attached)



repo.tar
Description: Unix tar archive



we have a test program with three translation units.  One has C++ methods and 
was built with debug info ("foo"), one has C++ methods and was built without 
debug info ("bar").  It tries calling these from lldb:


(lldb) p (void)foo('c')
in foo(char)
(lldb) p (void)foo(5)
in foo(int)
(lldb) p (void)foo(5, 5)
in foo(int, int)

We had debug info for foo(), called the correct methods.


(lldb) p (void)bar('c')
in bar(char *)
(lldb) p (void)bar(5)
in bar(char *)
(lldb) p (void)bar(5, 5)
in bar(char *)


Here, we have no debug info for bar(), so we grabbed the first bar() method we 
found and used it for all calls.  This is a problem.


(lldb) p (void)_Z3barc('c')
in bar(char)
(lldb) p (void)_Z3bari(5)
in bar(int)
(lldb) p (void)_Z3barii(5,5)
in bar(int, int)

Calling the mangled name bar()'s directly works as expected.



Davide is trying to solve that middle one.  I think the problem he was working 
on is where we had an expression using operator== and the first "decl" we found 
of this was in a no-debug-info translation unit, and that randomly chosen 
operator== was used when there WAS debug info for this operator== in a 
different translation unit in the process.

The question I have is -- should this even be allowed.  Should you be able to 
call a C++ method using a demangled name with no debug info?  I'm trying to 
think of a case where people do this today.  Clearly it does not work correctly 
today for overloaded functions.  And apparently this could be chosen over a 
debug info definition that might appear elsewhere in the process?  I didn't try 
to test that.

J___
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.

[Davide Italiano via lldb-commits]

On Tue, Mar 13, 2018 at 10:56 AM, Greg Clayton  wrote:
>
>
> On Mar 13, 2018, at 10:25 AM, Davide Italiano via lldb-commits
>  wrote:
>
> On Tue, Mar 13, 2018 at 9:59 AM, Frédéric Riss via lldb-commits
>  wrote:
>
>
>
> 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?
>
>
> Thank you all for your feedback. I'll reply with a single mail to everybody.
>
> C decls can be inserted. In fact, this works, even after my changes:
>
> (lldb) call printf("patatino")
> (int) $0 = 8
>
>
> That is because we define a function prototype for printf. So please try
> another function. You always have to cast the result of a function call to a
> symbol. In this case, since you didn't get a warning, you can know that you
> didn't use a symbol...
>
>
> I always thought identifiers beginning with underscore where illegal in C.
> Here's what the standard says:
>
> http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf
>
> Section 7.1.3
> "All identifiers that begin with an underscore and either an uppercase
> letter or another underscore are always reserved for any use."
>
>
> And yet there are many symbols that violate this. Just because someone says
> it is so, doesn't mean it is and also if there is no enforcement, then the
> rule doesn't mean much.
>
>
> They're not quite illegal, but they're reserved, so I'm unsure how
> frequent having symbols starting with `_Z` is popular.
>
>
> It is a legal C identifier, so we need to be able to call it.
>
>
> Maybe lldb has a better way of detecting whether this is a C or a C++
> program?
>
>
> Yes with debug info, no if we only have symbol table.
>
>
> There are several constraints:
>
> 1) The object from which we're loading symbols has no debug info, so
> we can't look at the CU and just say whether it's C or C++ or
> Objective-C.
> 2) The expression parser always evaluates expressions in a C++ context
> (to the best of my understanding)
> 3) You can always mix-and-match C/C++ object files as they're just
> Mach-O or ELF objects at that point (not recommended, but I've seen
> people doing it).
>
> Do you have any thoughts on how this should be achieved?
>
>
> What are you trying to achieve?
>

We had the report of a crash where lldb was setting a conditional
breakpoint on an invalid condition (CGRect == pointer, which is,
ill-formed).
The symbol-based resolution of lldb was picking a random operator==,
inserting a generic function decl operator== in the context because it
happened to find a matching symbol somewhere in the system.

clang expects operator== to have at least one argument, so you end up
hitting this assertion in Sema.

(lldb) Assertion failed: (i < getNumParams() && "Illegal param #"),
function getParamDecl, file
/Users/davide/work/llvm-monorepo/llvm-project-20170507/llvm/tools/clang/include/clang/AST/Decl.h,
line 2232.

So, to answer your question, Greg, I just want lldb to not inject
arbitrary C++ func decl. What do you think is the best way of
achieving this?

Best,

--
Davide
___
lldb-commits mailing list
lldb-commits@lists.llvm.org

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

[Davide Italiano via lldb-commits]

On Tue, Mar 13, 2018 at 11:17 AM, Greg Clayton  wrote:
> If you check the expression log, you will see the printf() function 
> declaration and any others we add:
>
> (lldb) log enable lldb expr
> (lldb) 2+3
>
> ...
> #ifndef NULL
> #define NULL (__null)
> #endif
> #ifndef Nil
> #define Nil (__null)
> #endif
> #ifndef nil
> #define nil (__null)
> #endif
> #ifndef YES
> #define YES ((BOOL)1)
> #endif
> #ifndef NO
> #define NO ((BOOL)0)
> #endif
> typedef __INT8_TYPE__ int8_t;
> typedef __UINT8_TYPE__ uint8_t;
> typedef __INT16_TYPE__ int16_t;
> typedef __UINT16_TYPE__ uint16_t;
> typedef __INT32_TYPE__ int32_t;
> typedef __UINT32_TYPE__ uint32_t;
> typedef __INT64_TYPE__ int64_t;
> typedef __UINT64_TYPE__ uint64_t;
> typedef __INTPTR_TYPE__ intptr_t;
> typedef __UINTPTR_TYPE__ uintptr_t;
> typedef __SIZE_TYPE__ size_t;
> typedef __PTRDIFF_TYPE__ ptrdiff_t;
> typedef unsigned short unichar;
> extern "C"
> {
> int printf(const char * __restrict, ...);
> }
>
> typedef signed char BOOL;
>
>
> void
> $__lldb_expr(void *$__lldb_arg)
> {
> ;
> /*LLDB_BODY_START*/
> 2+3;
> /*LLDB_BODY_END*/
> }
>
> ...
>

I checked it works also for strtok(), which I don't think we add.

Best,

--
Davide
___
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.

[Greg Clayton via lldb-commits]

If you check the expression log, you will see the printf() function declaration 
and any others we add:

(lldb) log enable lldb expr
(lldb) 2+3

...
#ifndef NULL
#define NULL (__null)
#endif
#ifndef Nil
#define Nil (__null)
#endif
#ifndef nil
#define nil (__null)
#endif
#ifndef YES
#define YES ((BOOL)1)
#endif
#ifndef NO
#define NO ((BOOL)0)
#endif
typedef __INT8_TYPE__ int8_t;
typedef __UINT8_TYPE__ uint8_t;
typedef __INT16_TYPE__ int16_t;
typedef __UINT16_TYPE__ uint16_t;
typedef __INT32_TYPE__ int32_t;
typedef __UINT32_TYPE__ uint32_t;
typedef __INT64_TYPE__ int64_t;
typedef __UINT64_TYPE__ uint64_t;
typedef __INTPTR_TYPE__ intptr_t;
typedef __UINTPTR_TYPE__ uintptr_t;
typedef __SIZE_TYPE__ size_t;
typedef __PTRDIFF_TYPE__ ptrdiff_t;
typedef unsigned short unichar;
extern "C"
{
int printf(const char * __restrict, ...);
}

typedef signed char BOOL;


void   
$__lldb_expr(void *$__lldb_arg)  
{  
;
/*LLDB_BODY_START*/
2+3;
/*LLDB_BODY_END*/
}  

...



> On Mar 13, 2018, at 11:04 AM, Davide Italiano  wrote:
> 
> On Tue, Mar 13, 2018 at 10:57 AM, Greg Clayton  wrote:
>> 
>> 
>>> On Mar 13, 2018, at 10:37 AM, Davide Italiano via lldb-commits 
>>>  wrote:
>>> 
>>> An alternative proposed by Fred which is an OK middle ground IMHO is
>>> that of not inserting a decl for the unmangled name, but treat this
>>> symbol always as-it-is.
>>> i.e. if we have a symbol _Znwm in some object, we don't insert a decl
>>> for `operator new(unsigned long)` but for _Znwm itself.
>>> 
>>> Greg, Jason, would that work for you guys?
>> 
>> I would be ok with that.
>> 
> 
> I'm going to send a review for this soon.
> 
> Best,
> 
> --
> Davide

___
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.

[Davide Italiano via lldb-commits]

On Tue, Mar 13, 2018 at 10:57 AM, Greg Clayton  wrote:
>
>
>> On Mar 13, 2018, at 10:37 AM, Davide Italiano via lldb-commits 
>>  wrote:
>>
>> An alternative proposed by Fred which is an OK middle ground IMHO is
>> that of not inserting a decl for the unmangled name, but treat this
>> symbol always as-it-is.
>> i.e. if we have a symbol _Znwm in some object, we don't insert a decl
>> for `operator new(unsigned long)` but for _Znwm itself.
>>
>> Greg, Jason, would that work for you guys?
>
> I would be ok with that.
>

I'm going to send a review for this soon.

Best,

--
Davide
___
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.

[Greg Clayton via lldb-commits]

> On Mar 13, 2018, at 10:37 AM, Davide Italiano via lldb-commits 
>  wrote:
> 
> An alternative proposed by Fred which is an OK middle ground IMHO is
> that of not inserting a decl for the unmangled name, but treat this
> symbol always as-it-is.
> i.e. if we have a symbol _Znwm in some object, we don't insert a decl
> for `operator new(unsigned long)` but for _Znwm itself.
> 
> Greg, Jason, would that work for you guys?

I would be ok with that.

> 
> Thanks!
> 
> --
> Davide
> 
> On Tue, Mar 13, 2018 at 10:25 AM, Davide Italiano  
> wrote:
>> On Tue, Mar 13, 2018 at 9:59 AM, Frédéric Riss via lldb-commits
>>  wrote:
>>> 
>>> 
 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?
>>> 
>> 
>> Thank you all for your feedback. I'll reply with a single mail to everybody.
>> 
>> C decls can be inserted. In fact, this works, even after my changes:
>> 
>> (lldb) call printf("patatino")
>> (int) $0 = 8
>> 
>> I always thought identifiers beginning with underscore where illegal in C.
>> Here's what the standard says:
>> 
>> http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf
>> 
>> Section 7.1.3
>> "All identifiers that begin with an underscore and either an uppercase
>> letter or another underscore are always reserved for any use."
>> 
>> They're not quite illegal, but they're reserved, so I'm unsure how
>> frequent having symbols starting with `_Z` is popular.
>> 
>> Maybe lldb has a better way of detecting whether this is a C or a C++ 
>> program?
>> 
>> There are several constraints:
>> 
>> 1) The object from which we're loading symbols has no debug info, so
>> we can't look at the CU and just say whether it's C or C++ or
>> Objective-C.
>> 2) The expression parser always evaluates expressions in a C++ context
>> (to the best of my understanding)
>> 3) You can always mix-and-match C/C++ object files as they're just
>> Mach-O or ELF objects at that point (not recommended, but I've seen
>> people doing it).
>> 
>> Do you have any thoughts on how this should be achieved?
>> 
>> Best,
>> 
>> --
>> Davide
> ___
> 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] [lldb] r327356 - [ExpressionParser] Fix crash when evaluating invalid expresssions.

[Greg Clayton via lldb-commits]

> On Mar 13, 2018, at 10:25 AM, Davide Italiano via lldb-commits 
>  wrote:
> 
> On Tue, Mar 13, 2018 at 9:59 AM, Frédéric Riss via lldb-commits
> > wrote:
>> 
>> 
>>> 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?
>> 
> 
> Thank you all for your feedback. I'll reply with a single mail to everybody.
> 
> C decls can be inserted. In fact, this works, even after my changes:
> 
> (lldb) call printf("patatino")
> (int) $0 = 8

That is because we define a function prototype for printf. So please try 
another function. You always have to cast the result of a function call to a 
symbol. In this case, since you didn't get a warning, you can know that you 
didn't use a symbol...

> 
> I always thought identifiers beginning with underscore where illegal in C.
> Here's what the standard says:
> 
> http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf 
> 
> 
> Section 7.1.3
> "All identifiers that begin with an underscore and either an uppercase
> letter or another underscore are always reserved for any use."

And yet there are many symbols that violate this. Just because someone says it 
is so, doesn't mean it is and also if there is no enforcement, then the rule 
doesn't mean much.

> 
> They're not quite illegal, but they're reserved, so I'm unsure how
> frequent having symbols starting with `_Z` is popular.

It is a legal C identifier, so we need to be able to call it.

> 
> Maybe lldb has a better way of detecting whether this is a C or a C++ program?

Yes with debug info, no if we only have symbol table.

> 
> There are several constraints:
> 
> 1) The object from which we're loading symbols has no debug info, so
> we can't look at the CU and just say whether it's C or C++ or
> Objective-C.
> 2) The expression parser always evaluates expressions in a C++ context
> (to the best of my understanding)
> 3) You can always mix-and-match C/C++ object files as they're just
> Mach-O or ELF objects at that point (not recommended, but I've seen
> people doing it).
> 
> Do you have any thoughts on how this should be achieved?

What are you trying to achieve?

> 
> Best,
> 
> --
> Davide
> ___
> 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] [lldb] r327356 - [ExpressionParser] Fix crash when evaluating invalid expresssions.

[Davide Italiano via lldb-commits]

An alternative proposed by Fred which is an OK middle ground IMHO is
that of not inserting a decl for the unmangled name, but treat this
symbol always as-it-is.
i.e. if we have a symbol _Znwm in some object, we don't insert a decl
for `operator new(unsigned long)` but for _Znwm itself.

Greg, Jason, would that work for you guys?

Thanks!

--
Davide

On Tue, Mar 13, 2018 at 10:25 AM, Davide Italiano  wrote:
> On Tue, Mar 13, 2018 at 9:59 AM, Frédéric Riss via lldb-commits
>  wrote:
>>
>>
>>> 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?
>>
>
> Thank you all for your feedback. I'll reply with a single mail to everybody.
>
> C decls can be inserted. In fact, this works, even after my changes:
>
> (lldb) call printf("patatino")
> (int) $0 = 8
>
> I always thought identifiers beginning with underscore where illegal in C.
> Here's what the standard says:
>
> http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf
>
> Section 7.1.3
> "All identifiers that begin with an underscore and either an uppercase
> letter or another underscore are always reserved for any use."
>
> They're not quite illegal, but they're reserved, so I'm unsure how
> frequent having symbols starting with `_Z` is popular.
>
> Maybe lldb has a better way of detecting whether this is a C or a C++ program?
>
> There are several constraints:
>
> 1) The object from which we're loading symbols has no debug info, so
> we can't look at the CU and just say whether it's C or C++ or
> Objective-C.
> 2) The expression parser always evaluates expressions in a C++ context
> (to the best of my understanding)
> 3) You can always mix-and-match C/C++ object files as they're just
> Mach-O or ELF objects at that point (not recommended, but I've seen
> people doing it).
>
> Do you have any thoughts on how this should be achieved?
>
> Best,
>
> --
> Davide
___
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.

[Davide Italiano via lldb-commits]

On Tue, Mar 13, 2018 at 9:59 AM, Frédéric Riss via lldb-commits
 wrote:
>
>
>> 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?
>

Thank you all for your feedback. I'll reply with a single mail to everybody.

C decls can be inserted. In fact, this works, even after my changes:

(lldb) call printf("patatino")
(int) $0 = 8

I always thought identifiers beginning with underscore where illegal in C.
Here's what the standard says:

http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf

Section 7.1.3
"All identifiers that begin with an underscore and either an uppercase
letter or another underscore are always reserved for any use."

They're not quite illegal, but they're reserved, so I'm unsure how
frequent having symbols starting with `_Z` is popular.

Maybe lldb has a better way of detecting whether this is a C or a C++ program?

There are several constraints:

1) The object from which we're loading symbols has no debug info, so
we can't look at the CU and just say whether it's C or C++ or
Objective-C.
2) The expression parser always evaluates expressions in a C++ context
(to the best of my understanding)
3) You can always mix-and-match C/C++ object files as they're just
Mach-O or ELF objects at that point (not recommended, but I've seen
people doing it).

Do you have any thoughts on how this should be achieved?

Best,

--
Davide
___
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.

[Greg Clayton via lldb-commits]

I agree with Jason, we shouldn't be looking at magic naming and making 
assumptions. What is the name is a C function that starts with _Z.

Both of Jason's examples below should work. Sometimes we only have symbol 
tables and if people are able to only get to the C++ function via a mangled 
name, they should be able to use it.

Greg


> On Mar 13, 2018, at 10:13 AM, Jason Molenda via lldb-commits 
>  wrote:
> 
> 
> 
>> On Mar 12, 2018, at 7:52 PM, Davide Italiano via lldb-commits 
>>  wrote:
>> 
>> On Mon, Mar 12, 2018 at 7:27 PM, Jason Molenda via lldb-commits
>>  wrote:
>>> FWIW, we'll definitely get pushback about
>>> 
 Now you can't do in lldb anymore
 (lldb) call _Zsomethingsomething(1,2,3)
>>> 
>>> 
>>> There are clever users of lldb, often working at assembly language level 
>>> without any symbolic information, who are able to correctly call a C++ 
>>> mangled function by hand.  It's not something I'd want to do personally, 
>>> but I'd be mad at my debugger if it gave me an error message when I told it 
>>> what to do.
>>> 
>>> 
>> 
>> I understand your point. We had several discussions about this and I
>> was really reluctant to push this change to begin with.
> 
> Sorry for missing out on the earlier discussions and coming in to this late.
> 
> 
> I don't understand exactly what's going on here.  If I have a program like
> 
> extern "C"  void takefive_c(int five) {};
> void takefive_cpp(int five) {}
> int main () {}
> 
> compiled without debug information, I can do
> 
> (lldb) p (void)takefive_c(5)
> (lldb) p (void)_Z12takefive_cppi(5)
> 
> 
> why is the second expression any different than the first?  I'm calling a 
> symbol name without any debug information, prototypes, or types at all.
> 
> 
> 
> 
>> I thought about alternatives and basically I ended up realizing that
>> maintaining the invariants that the AST wants Is crazytown.
>> You don't need to squint too hard to see that basically nobody using
>> clang does this.
>> All the clang tools prefer to insert text in a source file and reparse
>> it because modifying the AST in place is hard.
>> It's funny that they do that because the changes they make are
>> generally fairly localized, but nobody really understands what Sema
>> really wants (note, the invariants are hardcoded in the Sema path).
>> Injecting a generic decl in the context and crossing finger is not
>> really great IMHO. It causes crashes, which, from what I understand in
>> the lldb world are much worse than not being able to display
>> informations (from what I can see the mantra of lldb is "the debugger
>> should never crash").
>> 
>> So, yes, this patch is in some way picking one of two poisons. If we
>> want to take a look at this again (i.e. we consider it important
>> enough), then the interaction between debugger and compiler needs to
>> be rethought to inject something less terrible than a generic decl.
>> This is, needless to say, fairly hard because in this case you're
>> getting the symbols from a Mach-O object file, so all the state you
>> have is a mangled symbol name, which doesn't contain enough
>> information to reconstruct the whole context. In swift we can do
>> better because we have a fully-fledged type reconstruction mechanism
>> (swift::ide::GetTypeFromMangledSymbolName()), and even there, we
>> experienced a fair amount of pain because the path still has issues
>> that need to be shaken.
>> 
>> tl;dr if somebody wants to rewrite this, I'll be very happy to support
>> this , but I don't see a real reason to keep the feature in an
>> half-baked state.
>> 
>> Best,
>> 
>> --
>> Davide
>> ___
>> 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 
> 
___
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.

[Jason Molenda via lldb-commits]

> On Mar 12, 2018, at 7:52 PM, Davide Italiano via lldb-commits 
>  wrote:
> 
> On Mon, Mar 12, 2018 at 7:27 PM, Jason Molenda via lldb-commits
>  wrote:
>> FWIW, we'll definitely get pushback about
>> 
>>> Now you can't do in lldb anymore
>>> (lldb) call _Zsomethingsomething(1,2,3)
>> 
>> 
>> There are clever users of lldb, often working at assembly language level 
>> without any symbolic information, who are able to correctly call a C++ 
>> mangled function by hand.  It's not something I'd want to do personally, but 
>> I'd be mad at my debugger if it gave me an error message when I told it what 
>> to do.
>> 
>> 
> 
> I understand your point. We had several discussions about this and I
> was really reluctant to push this change to begin with.

Sorry for missing out on the earlier discussions and coming in to this late.


I don't understand exactly what's going on here.  If I have a program like

extern "C"  void takefive_c(int five) {};
void takefive_cpp(int five) {}
int main () {}

compiled without debug information, I can do

(lldb) p (void)takefive_c(5)
(lldb) p (void)_Z12takefive_cppi(5)


why is the second expression any different than the first?  I'm calling a 
symbol name without any debug information, prototypes, or types at all.




> I thought about alternatives and basically I ended up realizing that
> maintaining the invariants that the AST wants Is crazytown.
> You don't need to squint too hard to see that basically nobody using
> clang does this.
> All the clang tools prefer to insert text in a source file and reparse
> it because modifying the AST in place is hard.
> It's funny that they do that because the changes they make are
> generally fairly localized, but nobody really understands what Sema
> really wants (note, the invariants are hardcoded in the Sema path).
> Injecting a generic decl in the context and crossing finger is not
> really great IMHO. It causes crashes, which, from what I understand in
> the lldb world are much worse than not being able to display
> informations (from what I can see the mantra of lldb is "the debugger
> should never crash").
> 
> So, yes, this patch is in some way picking one of two poisons. If we
> want to take a look at this again (i.e. we consider it important
> enough), then the interaction between debugger and compiler needs to
> be rethought to inject something less terrible than a generic decl.
> This is, needless to say, fairly hard because in this case you're
> getting the symbols from a Mach-O object file, so all the state you
> have is a mangled symbol name, which doesn't contain enough
> information to reconstruct the whole context. In swift we can do
> better because we have a fully-fledged type reconstruction mechanism
> (swift::ide::GetTypeFromMangledSymbolName()), and even there, we
> experienced a fair amount of pain because the path still has issues
> that need to be shaken.
> 
> tl;dr if somebody wants to rewrite this, I'll be very happy to support
> this , but I don't see a real reason to keep the feature in an
> half-baked state.
> 
> Best,
> 
> --
> Davide
> ___
> 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] [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

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

[Davide Italiano via lldb-commits]

On Mon, Mar 12, 2018 at 7:27 PM, Jason Molenda via lldb-commits
 wrote:
> FWIW, we'll definitely get pushback about
>
>> Now you can't do in lldb anymore
>> (lldb) call _Zsomethingsomething(1,2,3)
>
>
> There are clever users of lldb, often working at assembly language level 
> without any symbolic information, who are able to correctly call a C++ 
> mangled function by hand.  It's not something I'd want to do personally, but 
> I'd be mad at my debugger if it gave me an error message when I told it what 
> to do.
>
>

I understand your point. We had several discussions about this and I
was really reluctant to push this change to begin with.
I thought about alternatives and basically I ended up realizing that
maintaining the invariants that the AST wants Is crazytown.
You don't need to squint too hard to see that basically nobody using
clang does this.
All the clang tools prefer to insert text in a source file and reparse
it because modifying the AST in place is hard.
It's funny that they do that because the changes they make are
generally fairly localized, but nobody really understands what Sema
really wants (note, the invariants are hardcoded in the Sema path).
Injecting a generic decl in the context and crossing finger is not
really great IMHO. It causes crashes, which, from what I understand in
the lldb world are much worse than not being able to display
informations (from what I can see the mantra of lldb is "the debugger
should never crash").

So, yes, this patch is in some way picking one of two poisons. If we
want to take a look at this again (i.e. we consider it important
enough), then the interaction between debugger and compiler needs to
be rethought to inject something less terrible than a generic decl.
This is, needless to say, fairly hard because in this case you're
getting the symbols from a Mach-O object file, so all the state you
have is a mangled symbol name, which doesn't contain enough
information to reconstruct the whole context. In swift we can do
better because we have a fully-fledged type reconstruction mechanism
(swift::ide::GetTypeFromMangledSymbolName()), and even there, we
experienced a fair amount of pain because the path still has issues
that need to be shaken.

tl;dr if somebody wants to rewrite this, I'll be very happy to support
this , but I don't see a real reason to keep the feature in an
half-baked state.

Best,

--
Davide
___
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.

[Jason Molenda via lldb-commits]

FWIW, we'll definitely get pushback about

> Now you can't do in lldb anymore
> (lldb) call _Zsomethingsomething(1,2,3)


There are clever users of lldb, often working at assembly language level 
without any symbolic information, who are able to correctly call a C++ mangled 
function by hand.  It's not something I'd want to do personally, but I'd be mad 
at my debugger if it gave me an error message when I told it what to do.


J



> 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.
> 
> 
> 
> 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