The C-string table and strcmp solution is fine, but I think the
StringSwitch method is strictly better. It's both faster (although I think
we agreed that speed doesn't matter in this case) //and// more readable.
Another alternative would be:
constexpr StringRef Registers[] = {"r12", "r13", "r14",
I don't think anyone disagrees with that -- the simple c-string table & strcmp
solution is fine. It's fun to muse about different approaches like this,
though. More generally, I think the way lldb handles registers is one of the
less well-designed parts of the debugger and it's something I oft
Unless it's showing up on a profile, isn't all of this just a solution
looking for a problem? Davide claims neither the original or updated code
shows up on any profiles, so why don't we just default to readable?
On Tue, Sep 5, 2017 at 3:17 PM Greg Clayton wrote:
> We should actually populate t
We should actually populate the register info with a cached value of this so we
do this once per process?
> On Sep 5, 2017, at 3:12 PM, Jason Molenda wrote:
>
> This method gets called every time we try to read a register in the unwinder
> when we're above stack frame 0. The unwinder won't tr
This method gets called every time we try to read a register in the unwinder
when we're above stack frame 0. The unwinder won't try to fetch volatile
(non-callee-spilled) registers, and it uses this ABI method to check before
trying to retrieve the reg.
We could switch the preserved register n
You could also use a collection of lldb_private::ConstString objects. Comparing
those are pointer compares since lldb_private::ConstString unique the strings
into a string pool. lldb_private::UniqueCStringMap has a very efficient way to
do this if you need an example.
Not sure how many times we
Ok last message. I bet it's because the patch writes std::string
Name(reg_info->Name);
change this to StringRef and it should be fine. I'd be curious to see how
many instructions that generates.
On Tue, Sep 5, 2017 at 2:12 PM Zachary Turner wrote:
> I noticed you said it generates new and del
I noticed you said it generates new and delete. I find that strange, we
should look into why that's happening because it's not supposed to be.
On Tue, Sep 5, 2017 at 2:07 PM Zachary Turner wrote:
> StringSwitch doesn't create any std::strings (doing so would allocate
> memory), but it does do t
StringSwitch doesn't create any std::strings (doing so would allocate
memory), but it does do the memcmp. Unless it's in a hot path I think
optimizing for readability is the right choice.
On Tue, Sep 5, 2017 at 2:02 PM Jason Molenda wrote:
>
>
> > On Sep 5, 2017, at 1:34 PM, Davide Italiano
>
On Tue, Sep 5, 2017 at 2:03 PM, Jason Molenda wrote:
>
>
>> On Sep 5, 2017, at 1:34 PM, Davide Italiano wrote:
>>
>> On Tue, Sep 5, 2017 at 1:23 PM, Jason Molenda wrote:
>>> Hi Davide, sorry I was offline for this discussion.
>>>
>>> I was a little curious about llvm::StringSwitch, I hadn't seen
> On Sep 5, 2017, at 1:34 PM, Davide Italiano wrote:
>
> On Tue, Sep 5, 2017 at 1:23 PM, Jason Molenda wrote:
>> Hi Davide, sorry I was offline for this discussion.
>>
>> I was a little curious about llvm::StringSwitch, I hadn't seen it before.
>> Is it creating std::string's for all of the
On Tue, Sep 5, 2017 at 1:23 PM, Jason Molenda wrote:
> Hi Davide, sorry I was offline for this discussion.
>
> I was a little curious about llvm::StringSwitch, I hadn't seen it before. Is
> it creating std::string's for all of these strings, then memcmp'ing the
> contents? Greg originally wrot
Hi Davide, sorry I was offline for this discussion.
I was a little curious about llvm::StringSwitch, I hadn't seen it before. Is
it creating std::string's for all of these strings, then memcmp'ing the
contents? Greg originally wrote these RegisterIsCalleeSaved() methods in the
ABI's hand opti
This revision was automatically updated to reflect the committed changes.
Closed by commit rL312501: [ABI] Rewrite RegisterIsCalleeSaved. (authored by
davide).
Changed prior to commit:
https://reviews.llvm.org/D37420?vs=113675&id=113790#toc
Repository:
rL LLVM
https://reviews.llvm.org/D3742
compnerd accepted this revision.
compnerd added a comment.
This revision is now accepted and ready to land.
Nice cleanup :-)
Comment at: source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp:1918
+.Cases("r12", "r13", "r14", "r15",
+ "rbp", "rbx", "ebp", "ebx", true)
davide added inline comments.
Comment at: source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp:1916
+ std::string Name = std::string(reg_info->name);
+ bool IsCalleeSaved = llvm::StringSwitch(Name)
+.Cases("r12", "r13", "r14", "r15",
Currently this uses two ca
davide created this revision.
The goal of this patch is twofold:
First, it removes a wrong comment (at least, not correctly describing what the
function does).
Then, it rewrites the function to use a stringswitch where the registers are
enumerated explicitly instead of being computed programmati
17 matches
Mail list logo
