[lldb-dev] Patch for addressing format warnings on 32-bit
There are a handful of -Wformat warnings on 32-bit platforms. I addressed all those that I’ve seen while working on Swift. Let me know if the git diff format is inappropriate for this. Cheers, - Will diff Description: Binary data ___ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
Re: [lldb-dev] Patch for addressing format warnings on 32-bit
> Message: 1
> Date: Sat, 26 Dec 2015 21:15:53 +0100
> From: Joerg Sonnenberger via lldb-dev
> To: lldb-dev@lists.llvm.org
> Subject: Re: [lldb-dev] Patch for addressing format warnings on 32-bit
> Message-ID: <20151226201553.gb14...@britannica.bec.de>
> Content-Type: text/plain; charset=utf-8
>
> On Fri, Dec 25, 2015 at 06:34:09PM -0800, William Dillon via lldb-dev wrote:
>> There are a handful of -Wformat warnings on 32-bit platforms.
>> I addressed all those that I’ve seen while working on Swift.
>> Let me know if the git diff format is inappropriate for this.
>
> Don't cast size_t to uint64_t, format it with %zu directly.
>
> Joerg
>
I can go ahead and do that, but I wonder whether there should be two different
ways of handling this, even on the same line. For example:
-error.SetErrorStringWithFormat ("SoftwareBreakpointr::%s addr=0x%"
PRIx64 ": tried to read %lu bytes but only read %" PRIu64, __FUNCTION__,
m_addr, m_opcode_size, (uint64_t)bytes_read);
+error.SetErrorStringWithFormat ("SoftwareBreakpointr::%s addr=0x%"
PRIx64 ": tried to read %" PRIu64 " bytes but only read %" PRIu64,
__FUNCTION__, m_addr, (uint64_t)m_opcode_size, (uint64_t)bytes_read);
- Will
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
Re: [lldb-dev] Patch for addressing format warnings on 32-bit
Hi Todd,
The example I put in my last email is one of a few (maybe one more) instances
where the existing code casts to 64-bit and uses PRIu64. When I’m dabbling in
existing code I try to copy the conventions that are already in place, so
that’s why I went this way. I’m happy to change it to %zu. I was just
checking about that.
- Will
> On Dec 28, 2015, at 10:24 AM, Todd Fiala wrote:
>
> Hi William,
>
> It looks like just the PRIx64/PRIu64 bits are needed from a visual
> inspection. The source variables that are printed from already are 64-bit
> always, aren't they? If they're not but they should be, that seems like the
> real underlying problem rather than needing to cast.
>
> What kind of warning are you seeing if you just replace the % format
> specifier?
>
> Thanks!
>
> -Todd
>
> On Sun, Dec 27, 2015 at 12:32 PM, William Dillon via lldb-dev
> mailto:lldb-dev@lists.llvm.org>> wrote:
> > Message: 1
> > Date: Sat, 26 Dec 2015 21:15:53 +0100
> > From: Joerg Sonnenberger via lldb-dev > <mailto:lldb-dev@lists.llvm.org>>
> > To: lldb-dev@lists.llvm.org <mailto:lldb-dev@lists.llvm.org>
> > Subject: Re: [lldb-dev] Patch for addressing format warnings on 32-bit
> > Message-ID: <20151226201553.gb14...@britannica.bec.de
> > <mailto:20151226201553.gb14...@britannica.bec.de>>
> > Content-Type: text/plain; charset=utf-8
> >
> > On Fri, Dec 25, 2015 at 06:34:09PM -0800, William Dillon via lldb-dev wrote:
> >> There are a handful of -Wformat warnings on 32-bit platforms.
> >> I addressed all those that I’ve seen while working on Swift.
> >> Let me know if the git diff format is inappropriate for this.
> >
> > Don't cast size_t to uint64_t, format it with %zu directly.
> >
> > Joerg
> >
>
> I can go ahead and do that, but I wonder whether there should be two
> different ways of handling this, even on the same line. For example:
>
> -error.SetErrorStringWithFormat ("SoftwareBreakpointr::%s
> addr=0x%" PRIx64 ": tried to read %lu bytes but only read %" PRIu64,
> __FUNCTION__, m_addr, m_opcode_size, (uint64_t)bytes_read);
> +error.SetErrorStringWithFormat ("SoftwareBreakpointr::%s
> addr=0x%" PRIx64 ": tried to read %" PRIu64 " bytes but only read %" PRIu64,
> __FUNCTION__, m_addr, (uint64_t)m_opcode_size, (uint64_t)bytes_read);
>
> - Will
> ___
> lldb-dev mailing list
> lldb-dev@lists.llvm.org <mailto:lldb-dev@lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
> <http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev>
>
>
>
> --
> -Todd
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
[lldb-dev] Patch to fix REPL for ARMv7 & ARMv6 on linux
Hi All, There is a small change that enables correct calculation of arm sub architectures while using the REPL on arm-linux. As you may of may or may not know, linux appends ‘l’ to arm architecture versions to denote little endian. This sometimes interferes with the determination of the architecture in the triple. I experimented with adding sub architecture entries for these within lldb, but I discovered a simpler (and less invasive) method. Because LLVM already knows how to handle some of these cases (I have a patch submitted for review that enables v6l; v7l already works), I am relying on llvm to clean it up. The gist of it is that the llvm constructor (when given a triple string) retains the provided string unless an accessor mutates it. Meanwhile, the accessors for the components go through the aliasing and parsing logic. This code detects whether the sub-architecture that armv6l or armv7l aliases to is detected, and re-sets the architecture in the triple. This overwrites the architecture that comes from linux, thus sanitizing it. Some kind of solution is required for the REPL to on arm-linux. Thoughts? - Will lldb.diff Description: Binary data ___ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
[lldb-dev] Canonicalize armv6l and armv7l to v6 and v7
Hi all, I have a small patch that fixes an issue that prevents use of the Swift REPL on arm-linux. When working with targets that the OS presents as armv7l, for example, there is an inconsistency when the architecture sub-type is parsed. Detecting this condition and removing the ‘l’ fixes it. I’ve tried to make this logic as restrictive in scope and application as possible. It will only trigger on arm hosts in linux. The theory of operation is that, within llvm, the v6l and v7l subtypes alias to v6 and v7 respectively. Also in llvm, when a triple is created, the original string is preserved until parts of it are mutated. When llvm executes its architecture synonym logic, armv7l (and armv6l once my patch there is merged) is resolved to armv7. By querying the parsed subtype from llvm, this centralized logic is used to sanitize the architecture. The end result is that if the architecture provided to the triple constructor is an alias for the v7 or v6 subtype, it is discarded and replaced with v6 or v7 itself. My hope is that this code can make it into the Swift 2.2 release, and I’m working with Bob Wison on a related patch to LLVM. I appreciate any and all comments and suggestions. Thanks!! - Will lldb.diff Description: Binary data ___ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
Re: [lldb-dev] Patch to fix REPL for ARMv7 & ARMv6 on linux
Hi again, everyone I’d like to ping on this patch now that the 3.8 branch is fairly new, and merging it over is fairly straight-forward. Thanks in advance for your comments! - Will > There is a small change that enables correct calculation of arm sub > architectures while using the REPL on arm-linux. As you may of may or may > not know, linux appends ‘l’ to arm architecture versions to denote little > endian. This sometimes interferes with the determination of the architecture > in the triple. I experimented with adding sub architecture entries for these > within lldb, but I discovered a simpler (and less invasive) method. Because > LLVM already knows how to handle some of these cases (I have a patch > submitted for review that enables v6l; v7l already works), I am relying on > llvm to clean it up. The gist of it is that the llvm constructor (when given > a triple string) retains the provided string unless an accessor mutates it. > Meanwhile, the accessors for the components go through the aliasing and > parsing logic. This code detects whether the sub-architecture that armv6l or > armv7l aliases to is detected, and re-sets the architecture in the triple. > This overwrites the architecture that comes from linux, thus sanitizing it. > > Some kind of solution is required for the REPL to work on arm-linux. Without > it, the REPL crashes. lldb.diff Description: Binary data ___ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
Re: [lldb-dev] Patch to fix REPL for ARMv7 & ARMv6 on linux
Hi Omair,
In a very real sense, no information is lost here. The addition of the ‘l’
only indicates that the system is little endian. When the triple is created,
the flag setting little endian is set (and defaults to little anyway). There
is no other valid ARM sub architecture with ARMv6 or ARMv7 that ends with an
‘l’.
I based this change somewhat on the existing model for massaging LLVM triples
such as in HostInfoLinux.cpp:
void
HostInfoLinux::ComputeHostArchitectureSupport(ArchSpec &arch_32, ArchSpec
&arch_64)
{
HostInfoPosix::ComputeHostArchitectureSupport(arch_32, arch_64);
const char *distribution_id = GetDistributionId().data();
// On Linux, "unknown" in the vendor slot isn't what we want for the default
// triple. It's probably an artifact of config.guess.
if (arch_32.IsValid())
{
arch_32.SetDistributionId(distribution_id);
if (arch_32.GetTriple().getVendor() == llvm::Triple::UnknownVendor)
arch_32.GetTriple().setVendorName(llvm::StringRef());
}
if (arch_64.IsValid())
{
arch_64.SetDistributionId(distribution_id);
if (arch_64.GetTriple().getVendor() == llvm::Triple::UnknownVendor)
arch_64.GetTriple().setVendorName(llvm::StringRef());
}
}
Would it make you more comfortable is this patch was rewritten within
HostInfoLinux::ComputeHostArchitectureSupport alongside the massaging of the
vendor name for linux?
- Will
> On Jan 27, 2016, at 1:28 AM, Omair Javaid wrote:
>
> Hi Will,
>
> I dont understand REPL and thus the benefits it will have by making
> change to architecture name. I would not recommend to drop any
> information that we get from the host operating system.
>
> LLDB maintains core information alongwith triple in ArchSpec, may be
> you can parse triple to reflect correct core and use core instead of
> architecture name where needed.
>
> Kindly elaborate in a bit detail what are we getting out of this
> change for more accurate comments.
>
> Thanks!
>
> On 26 January 2016 at 14:47, Pavel Labath wrote:
>> + Omair
>>
>> I don't really understand arm (sub)-architectures or REPL. The patch
>> seems mostly harmless, but it also feels like a hack to me. A couple
>> of questions:
>> - why does this only pose a problem for REPL?
>> - If I understand correctly, the problem is that someone is looking at
>> the architecture string contained in the Triple, and not finding what
>> it expects. Is that so? Could you point me to (some of) the places
>> that do that.
>>
>> Omair, any thoughts on this?
>>
>> cheers,
>> pl
>>
>>
>> On 25 January 2016 at 18:55, Hans Wennborg wrote:
>>> This patch looks reasonable to me, but I don't know enough about LLDB
>>> to actually review it.
>>>
>>> +Renato or Pavel maybe?
>>>
>>> On Thu, Jan 14, 2016 at 11:32 AM, William Dillon via lldb-dev
>>> wrote:
>>>> Hi again, everyone
>>>>
>>>> I’d like to ping on this patch now that the 3.8 branch is fairly new, and
>>>> merging it over is fairly straight-forward.
>>>>
>>>> Thanks in advance for your comments!
>>>> - Will
>>>>
>>>>> There is a small change that enables correct calculation of arm sub
>>>>> architectures while using the REPL on arm-linux. As you may of may or
>>>>> may not know, linux appends ‘l’ to arm architecture versions to denote
>>>>> little endian. This sometimes interferes with the determination of the
>>>>> architecture in the triple. I experimented with adding sub architecture
>>>>> entries for these within lldb, but I discovered a simpler (and less
>>>>> invasive) method. Because LLVM already knows how to handle some of these
>>>>> cases (I have a patch submitted for review that enables v6l; v7l already
>>>>> works), I am relying on llvm to clean it up. The gist of it is that the
>>>>> llvm constructor (when given a triple string) retains the provided string
>>>>> unless an accessor mutates it. Meanwhile, the accessors for the
>>>>> components go through the aliasing and parsing logic. This code detects
>>>>> whether the sub-architecture that armv6l or armv7l aliases to is
>>>>> detected, and re-sets the architecture in the triple. This overwrites
>>>>> the architecture that comes from linux, thus sanitizing it.
>>>>>
>>>>> Some kind of solution is required for the REPL to work on arm-linux.
>>>>> Without it, the REPL crashes.
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
