[Lldb-commits] [PATCH] D49740: Move RegisterValue, Scalar, State from Core to Utility

Mon Aug 6 11:50:37 PDT 2018

jingham added a comment.

In https://reviews.llvm.org/D49740#1188587, @labath wrote:

> In https://reviews.llvm.org/D49740#1188055, @jingham wrote:
>
> > First off, I'm fine with Zachary's suggestion that we let the dust settle a bit before we try to organize things better.  We'll probably do a better job then.
>
>
> Thanks. I agree this can wait, but I also do have some ideas below on what could done differently right now for the Scalar and State classes. Let me know if you think I should try that instead. (Otherwise, I'll just go ahead with this.)

It doesn't seem like you're doing anything that is going to be hard to undo, so I'm fine with just doing this as you have it for now.  Maybe see if SafeMachO can go in Host, but that's the only one that seems easy and not requiring deeper thought.

> 
> 
>> But just to use these cases, for instance, Scalar is the base of how we realize values in the debugger (ValueObject lives on top of this).  It would be good if its landing place was such that if I was looking at how we represent values it was logically placed for that.
> 
> I think putting Scalar next to the ValueObject stuff would make sense, if the ValueObjects were the only user of this class. However,  that isn't the case right now. The reason I am touching this class in the first place is that it is being used from within the RegisterValue class. This means it is more than just a ValueObject helper, but more like a self-contained "utility" which can be reused in different contexts.
> 
> Now, I am not actually sure whether using Scalar within RegisterValue is a good idea (seems a bit overkill for that), but it seemed to work, so I didn't want to disturb that (and I do believe that Scalar could be useful outside of the ValueObject hierarchy). However, I can take another look at what it would take to stop using Scalar in the context of RegisterValues, which would free us up to move RegisterValue without touching the Scalar class.

I'm not sure how much of Scalar RegisterValue needs when it isn't being used to back a ValueObjectRegister.  Scalar is a pretty ambitious class, since it supports everything you would need to back DWARF expression manipulations.  RegisterValue does get used in a bunch of places stand alone, though a lot of that just seems to be setting or dumping.

But I guess I'm looking for an organization based on "groups by functionality" rather than "who uses what".  So even though other actors might use Scalar, it's reason for being is to present values.  For the purposes of understanding the code, grouping all the files that work together for a task seems to me like it would help.  But again, this is more in the service of people newer to the code.

> 
> 
>> State is part of how we present the Process current state, so it seems like it should be grouped with other process related  entities.
> 
> State is a bit funny. It is currently used from both liblldb and lldb-server, but these use different hierarchies for "process-related entities", so that's why I'm moving it to a common place. However, I can certainly see a case for LLGS and liblldb having different State enums. Sharing it doesn't buy us much (ATM it's just the StateAsCString function), and using a different enum in LLGS would have another benefit. Right now it only uses a subset of all State values, so using an different enum would allow us to capture the possible states more precisely. Doing that instead of moving State.h around should be easy enough.
> 
>>   And RegisterValue belongs with the other parts of lldb that take apart the machine level state of a process.
> 
> I am not sure which parts are *you* thinking about here, but I think it would be nice to have this class together with all the definitions of RegisterInfo structs and associated constants. These are now mostly in Plugin/Process/Utility, but so is a lot of other stuff. At one point I will get around to worrying about those too, so maybe then we could move all of these things to a separate module.
> 
>> It will probably go away, but FastDemangle really belongs around the language handling code.  I agree that Utility is an odd place for CompletionRequest...
>> 
>> SafeMachO is weird.  It gets used in Host - both common & macosx, and we're trying not to include out of Plugins so the MachO object file plugin directory doesn't seem right.  Maybe Host is a better place, it's reason for being is so you can include both llvm/Support/MachO.h and mach/machine.h, so even though it's not directly a host functionality it look a bit like that.  Not sure.
> 
> I think Host makes sense. None of the other code (except the NativeProcessXXX classes, I guess) should really be including OS-specific stuff, so there shouldn't be a need (in an ideal world, I don't know how far we are from it right now) for this header anywhere except Host code.

Yes, that makes sense to me.

>> There's also tension between "things that belong together functionally" and "things that need to be separated because we want to layer one strictly on top of the other, since we seem to be treating directories as a representation of dependencies.  Do we want to have a Values directory with ValueObject at the top level, and Scalar in a no-dependency subdirectory?
>> 
>> TTTT, if I need to find a file these days, I either Cmd-Click on a type to go to its definition, or type Cmd-Shift-O and start typing some bits of the file name and Xcode finds it for me.  I'm pretty familiar with the code, so I don't need a higher level skeleton to help me figure out what all is in this project.  I think at this point many of us are in this state...
>> 
>> But once you get past the build issues, the real audience for this level of organization is folks coming new to the project.  It would be good to hear from some of the newer folks what they found confusing or what would have helped them navigate around the project as they are starting out.
> 
> Yes, that's certainly a good point. I'd like to hear that too.

https://reviews.llvm.org/D49740