krytarowski added inline comments.

================
Comment at: source/Plugins/Process/elf-core/elf-core-enums.h:14
+/// Core files PT_NOTE segment descriptor types
+enum {
+  NT_PRSTATUS = 1,
----------------
labath wrote:
> krytarowski wrote:
> > alexandreyy wrote:
> > > krytarowski wrote:
> > > > alexandreyy wrote:
> > > > > krytarowski wrote:
> > > > > > No namespace here?
> > > > > I think these constants are used for multiple OSs.
> > > > > Am I correct?
> > > > > Do you have a suggestion for a namespace?
> > > > There is rumor that they came from SVR4.. but I don't have the specs to 
> > > > make sure.
> > > > 
> > > > Multiple in this case does not mean "all". This is not true for at 
> > > > least NetBSD.
> > > I can remove these constants and modify the first "if" in 
> > > ParseThreadContextsFromNoteSegment to:
> > > 
> > >   if (((note.n_type == LINUX::NT_PRSTATUS ||
> > >           note.n_type == FREEBSD::NT_PRSTATUS) && have_prstatus) ||
> > >         ((note.n_type == LINUX::NT_PRPSINFO ||
> > >           note.n_type == FREEBSD::NT_PRPSINFO) && have_prpsinfo)) {
> > >       assert(thread_data->gpregset.GetByteSize() > 0);
> > > 
> > > What do you think?
> > > But NT_PRSTATUS and NT_PRPSINFO have the same value for Linux and FreeBSD 
> > > .
> > I propose to put NT_PRSTATUS, NT_PRFPREG, NT_PRPSINFO into this namespace 
> > and call it SVR4.
> > 
> > SmartOS uses the same values.
> With the addition of all the bsd variants, the note parsing code has grown 
> pretty big.
> 
> My plan was to refactor it a bit after this is committed. I wanted to split 
> it into two stages: first we just determine the OS from the notes; and then 
> we dispatch to an appropriate os-specific function to do the actual parsing 
> (with each function just using the NT constants from its own namespace). If 
> you agree with that direction then I propose to go with the 
> LINUX::NT_PRSTATUS || FREEBSD::NT_PRSTATUS proposed by alexandreyy in the 
> interim.
Sounds good.


https://reviews.llvm.org/D39681



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

Reply via email to