ted added inline comments.

================
Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4357
 
         assert(reg_info.byte_size != 0);
         registers.push_back(reg_info);
----------------
labath wrote:
> omjavaid wrote:
> > labath wrote:
> > > mgorny wrote:
> > > > omjavaid wrote:
> > > > > mgorny wrote:
> > > > > > omjavaid wrote:
> > > > > > > @mgorny the assert already exists but then we also want to allow 
> > > > > > > bit sized registers although they ll be viewed as byte length for 
> > > > > > > now.
> > > > > > Ah, right. I suppose you could skip zero-byte registers though. 
> > > > > > That should amend the assert with better release behavior.
> > > > > on a second thought, I dont see a zero sized register being sent by 
> > > > > stub as a big enough reason to abort the whole debug session unless 
> > > > > its one of GPRs. May be we skip the assert altogether and replace it 
> > > > > with an error message. 
> > > > > What do you think?
> > > > Yes, you are correct. Probably `LLDB_LOG` would go in line with how we 
> > > > handle these things.
> > > Yeah, I don't think crashing is a good response to the stub sending us 
> > > nonsensical register definitions. Though that seems like a separate 
> > > issue..
> > LLDB_LOG will hide message from user unless log is enabled. I think user 
> > must be notified that register is  zero sized and thats why you wont be 
> > able to see it in register read. Similar to the way we notify user about 
> > unhandled register attibutes like "save-restore".
> BTW, I came very close to deleting that printf when I was touching this code 
> last month.
> FWIW, my hierarchy is:
> user-facing warning (though I don't know how would that be implemented here) 
> >> log entry >> nothing >> raw printf
+1 on not crashing. The remote sending us garbage shouldn't cause us to crash - 
we should try to make sense of the garbage, and if we can't, error out 
gracefully. Better to print an error and ignore the register than crash or 
abort the session.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111131/new/

https://reviews.llvm.org/D111131

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

Reply via email to