3:54 a.m.
Hello,
patch attached. Please consider to add the changes mainline. But
please also check back all changes.
Some comments:
gdb/dwarf2-frame.c:
- I think the buf += overwrites the ++, at least my tests with
some test codes showed that.
tools.c:
- this is really strange, if index = 0, then the assignment
doesn't make sense. If it's random, it also doesn't make sense. :)
Didn't have time to dig into the whole logic of this hashtable.
Regards,
Bernhard
8:48 a.m.
Bernhard Walle wrote:
Hello,
patch attached. Please consider to add the changes mainline. But
please also check back all changes.
Some comments:
gdb/dwarf2-frame.c:
- I think the buf += overwrites the ++, at least my tests with
some test codes showed that.
tools.c:
- this is really strange, if index = 0, then the assignment
doesn't make sense. If it's random, it also doesn't make sense. :)
Didn't have time to dig into the whole logic of this hashtable.
Regards,
Bernhard
Hi Bernhard,
Thanks -- comments on each patch below...
--------------------------------
gdb-6.1.patch:
else if (*augmentation == 'P')
{
/* Skip. */
- buf += size_of_encoded_value (*buf++);
+ buf += size_of_encoded_value (*buf);
augmentation++;
}
This patch makes me a litte nervous. I see that gdb 6.5 does this:
else if (*augmentation == 'P')
{
/* Skip. Avoid indirection since we throw away the result. */
gdb_byte encoding = (*buf++) & ~DW_EH_PE_indirect;
read_encoded_value (unit, encoding, buf, &bytes_read);
buf += bytes_read;
augmentation++;
}
Are they equivalent?
--------------------------------
lkcd_common.c:
int
lkcd_lseek(physaddr_t paddr)
{
- long i;
+ long i = 0;
int err;
int eof;
void *dp;
Although it looks like it really doesn't matter where "i" starts,
this is fine.
--------------------------------
symbols.c:
ulong start, end;
char *modbuf;
ulong maxchunk, alloc;
- long offset;
+ long offset = 0;
That's fine...
--------------------------------
task.c:
void
dump_task_table(int verbose)
{
- int i, nr_cpus;
+ int i, nr_cpus = 1;
struct task_context *tc;
char buf[BUFSIZE];
int others, wrap, flen;
Actually that's a bug -- the subsequent assignment should read:
nr_cpus = kt->kernel_NR_CPUS ? kt->kernel_NR_CPUS : NR_CPUS;
--------------------------------
tools.c:
eval_common(char *s, int flags, int *errptr, struct number_option *np)
{
char *p1, *p2;
- char *op, opcode;
+ char *op, opcode = 0;
ulong value1;
ulong value2;
ulonglong ll_value1;
That's good...
@@ -3672,7 +3672,7 @@ hq_entry_exists(ulong value)
struct hash_table *ht;
struct hq_entry *list_entry;
long hqi;
- long index;
+ long index = 0;
Good point on this one. That function is never called by anybody,
but was added upon request of an extension writer. It's just
walking the entries in a hash list looking for a matching value
in an entry, and shouldn't be writing anything into an entry (and
certainly not an uninitialized anything!). In any case, there is
no need for the index variable to exist at all, so I'll just rip
it out.
Thanks,
Dave
1:04 p.m.
Hello,
* Dave Anderson <anderson(a)redhat.com> [2007-01-17 15:48]:
gdb-6.1.patch:
else if (*augmentation == 'P')
{
/* Skip. */
- buf += size_of_encoded_value (*buf++);
+ buf += size_of_encoded_value (*buf);
augmentation++;
}
This patch makes me a litte nervous. I see that gdb 6.5 does this:
else if (*augmentation == 'P')
{
/* Skip. Avoid indirection since we throw away the result. */
gdb_byte encoding = (*buf++) & ~DW_EH_PE_indirect;
read_encoded_value (unit, encoding, buf, &bytes_read);
buf += bytes_read;
augmentation++;
}
Are they equivalent?
Of course not. :) After thinking a bit I think the correct fix would
be:
buf += size_of_encoded_value(*buf);
buf++;
Do you aggree?
Attached I have some more fixes for the GDB patch (so this is no patch
for crash but should be added to gdb-6.1.patch). I saw that there's
some internal memset() with different paramter order in GDB [1], but
that's not taken in the gdb/ subdirectory because if you compile the
whole GDB with -D_FORTIFY_SOURCE=2, you get a linker (and not
compiler) warning. And the linker should know about it.
Regards,
Bernhard
[1] libiberty/memset.c
2:10 p.m.
Bernhard Walle wrote:
Hello,
* Dave Anderson <anderson(a)redhat.com> [2007-01-17 15:48]:
>
> gdb-6.1.patch:
>
> else if (*augmentation == 'P')
> {
> /* Skip. */
> - buf += size_of_encoded_value (*buf++);
> + buf += size_of_encoded_value (*buf);
> augmentation++;
> }
>
> This patch makes me a litte nervous. I see that gdb 6.5 does this:
>
> else if (*augmentation == 'P')
> {
> /* Skip. Avoid indirection since we throw away the result. */
> gdb_byte encoding = (*buf++) & ~DW_EH_PE_indirect;
> read_encoded_value (unit, encoding, buf, &bytes_read);
> buf += bytes_read;
> augmentation++;
> }
>
> Are they equivalent?
Of course not. :) After thinking a bit I think the correct fix would
be:
buf += size_of_encoded_value(*buf);
buf++;
Do you aggree?
I can't say with any confidence. I was trying to correlate the difference
between the way it's done in gdb-6.5 vs 6.1, and got lost doing so...
Just curious -- how do you manage to get crash to run through that
code path? (I put an exit() there, but can't seem to get there...)
Attached I have some more fixes for the GDB patch (so this is no
patch
for crash but should be added to gdb-6.1.patch). I saw that there's
some internal memset() with different paramter order in GDB [1], but
that's not taken in the gdb/ subdirectory because if you compile the
whole GDB with -D_FORTIFY_SOURCE=2, you get a linker (and not
compiler) warning. And the linker should know about it.
These are all fine -- they've been addressed similarly in gdb-6.5.
Thanks,
Dave
7:50 a.m.
Hi,
* Dave Anderson <anderson(a)redhat.com> [2007-01-17 21:10]:
Bernhard Walle wrote:
> >
> > This patch makes me a litte nervous. I see that gdb 6.5 does this:
> >
> > else if (*augmentation == 'P')
> > {
> > /* Skip. Avoid indirection since we throw away the result. */
> > gdb_byte encoding = (*buf++) & ~DW_EH_PE_indirect;
> > read_encoded_value (unit, encoding, buf, &bytes_read);
> > buf += bytes_read;
> > augmentation++;
> > }
> >
> > Are they equivalent?
>
> Of course not. :) After thinking a bit I think the correct fix would
> be:
>
> buf += size_of_encoded_value(*buf);
> buf++;
>
> Do you aggree?
I can't say with any confidence. I was trying to correlate the difference
between the way it's done in gdb-6.5 vs 6.1, and got lost doing so...
Yes, but 6.5 basically does:
gdb_byte tmp = *buf++;
buf += size_of_encoded_value(tmp);
which should be equivalent to
buf += size_of_encoded_value(*buf);
buf++;
Of course, read_encoded_value() does more than just calling
size_of_encoded_value(), but anyway.
Just curious -- how do you manage to get crash to run through that
code path? (I put an exit() there, but can't seem to get there...)
I didn't. I just wrote a small C program which has the same problem.
(Increasing that pointer in the same operation.)
Probably that code path will be never hit at all. ;)
BTW: Is it planned to upgrade crash to use GDB 6.6 or some higher
version?
Regards,
Bernhard
8:22 a.m.
Bernhard Walle wrote:
Hi,
* Dave Anderson <anderson(a)redhat.com> [2007-01-17 21:10]:
> Bernhard Walle wrote:
> > >
> > > This patch makes me a litte nervous. I see that gdb 6.5 does this:
> > >
> > > else if (*augmentation == 'P')
> > > {
> > > /* Skip. Avoid indirection since we throw away the result.
*/
> > > gdb_byte encoding = (*buf++) & ~DW_EH_PE_indirect;
> > > read_encoded_value (unit, encoding, buf, &bytes_read);
> > > buf += bytes_read;
> > > augmentation++;
> > > }
> > >
> > > Are they equivalent?
> >
> > Of course not. :) After thinking a bit I think the correct fix would
> > be:
> >
> > buf += size_of_encoded_value(*buf);
> > buf++;
> >
> > Do you aggree?
>
> I can't say with any confidence. I was trying to correlate the difference
> between the way it's done in gdb-6.5 vs 6.1, and got lost doing so...
Yes, but 6.5 basically does:
gdb_byte tmp = *buf++;
buf += size_of_encoded_value(tmp);
which should be equivalent to
buf += size_of_encoded_value(*buf);
buf++;
Of course, read_encoded_value() does more than just calling
size_of_encoded_value(), but anyway.
> Just curious -- how do you manage to get crash to run through that
> code path? (I put an exit() there, but can't seem to get there...)
I didn't. I just wrote a small C program which has the same problem.
(Increasing that pointer in the same operation.)
Probably that code path will be never hit at all. ;)
Ok then, I trust your "equivalency" explanation, and since in all
probability the code path won't be hit, it should be safe.
In fact, I don't believe that any of the other patches that you
sent will ever be exercised by crash either? But, if they make
your compiler happy, that's fine with me...
BTW: Is it planned to upgrade crash to use GDB 6.6 or some higher
version?
Only when and if it becomes absolutely necessary. Every time I've
done an upgrade, it's been a nightmare. The hooks that are there to
make back-door entries into the gdb code for the crash utility's purposes
never seem to port cleanly. And after the upgrades, there have always
been new issues that have cropped up -- in fact there was a time where
I had to revert back to gdb-6.0 from gdb-6.1.
And given that the gdb module is only utilized as "gdb vmlinux", as
long as gdb-6.1 is capable of returning structure sizes, member offsets,
code disassembly, etc., then there's no compelling need to upgrade it.
And if there are, the first line of defense would be to patch the current
version than make a wholesale upgrade.
Thanks,
Dave
9:30 a.m.
Hello,
* Dave Anderson <anderson(a)redhat.com> [2007-01-18 15:22]:
Ok then, I trust your "equivalency" explanation, and since
in all
probability the code path won't be hit, it should be safe.
In fact, I don't believe that any of the other patches that you
sent will ever be exercised by crash either?
I didn't check it. I just fixed compiler warnings.
But, if they make your compiler happy, that's fine with me...
It's your compiler, too. :) Yes, maybe you use another $RPM_OPT_FLAGS
as we do ...
And given that the gdb module is only utilized as "gdb
vmlinux", as
long as gdb-6.1 is capable of returning structure sizes, member offsets,
code disassembly, etc., then there's no compelling need to upgrade it.
And if there are, the first line of defense would be to patch the current
version than make a wholesale upgrade.
Sounds reasonable.
Regards,
Bernhard
6555
days inactive
6556
days old
devel@lists.crash-utility.osci.io
6 comments
2 participants
participants (2)
-
Bernhard Walle -
Dave Anderson