| Message ID | 1491586736-21296-3-git-send-email-arnez@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Andreas Arnez <arnez@linux.vnet.ibm.com> writes: > A field in a structure composed of DWARF pieces overlaps one or more of > those pieces. When writing to the field, the beginning of the first and > the end of the last of those pieces may have to be skipped. But the Do you have an example? > logic in write_pieced_value for handling this is flawed when there are > actually bits to skip at the beginning of the first piece: it truncates > the piece size towards the end *before* accounting for the skipped bits > at the beginning instead of the other way around. > > Note that the same bug was already found in read_pieced_value and fixed > there (but not in write_pieced_value), see PR 15391. Can we share the code in write_pieced_value and read_pieced_value? The code computing offsets and bits should be shared. Also, we need more comments in the code to explain these offsets and bits, a diagram about the relationships of these bits and offsets is quite helpful.
On Thu, Apr 13 2017, Yao Qi wrote: > Andreas Arnez <arnez@linux.vnet.ibm.com> writes: > >> A field in a structure composed of DWARF pieces overlaps one or more of >> those pieces. When writing to the field, the beginning of the first and >> the end of the last of those pieces may have to be skipped. But the > > Do you have an example? OK, I guess the commit message should be improved a bit. How about this? A field f in a structure composed of DWARF pieces may be located in multiple pieces, where the first and last of those may contain bits from other fields as well. So when writing to f, the beginning of the first and the end of the last of those pieces may have to be skipped. But the logic in write_pieced_value for handling one of those pieces is flawed when the first and last piece are the same, i.e., f is contained in a single piece: < - - - - - - - - - piece_size - - - - - - - - - -> +-------------------------------------------------+ | skipped_bits | f_bits | / / / / / / / / / / | +-------------------------------------------------+ The current logic determines the size of the sub-piece to operate on by limiting the piece size to the bit size of f and then subtracting the skipped bits: max (piece_size, f_bits) - skipped_bits Instead of: max (piece_size - skipped_bits, f_bits) So the resulting sub-piece size is corrupted, leading to wrong handling of this piece in write_pieced_value. > >> logic in write_pieced_value for handling this is flawed when there are >> actually bits to skip at the beginning of the first piece: it truncates >> the piece size towards the end *before* accounting for the skipped bits >> at the beginning instead of the other way around. >> >> Note that the same bug was already found in read_pieced_value and fixed >> there (but not in write_pieced_value), see PR 15391. > > Can we share the code in write_pieced_value and read_pieced_value? The > code computing offsets and bits should be shared. Yes. I have another patch (not posted yet) that merges these two functions. I moved that towards the end of the patch series, so the individual fixes can be incremental. > Also, we need more comments in the code to explain these offsets and > bits, a diagram about the relationships of these bits and offsets is > quite helpful. OK. Some of the offset variables are removed by my patches, so I guess I'll postpone that to the merged version. I'll see what I can come up with and include it in v2. -- Andreas
Andreas Arnez <arnez@linux.vnet.ibm.com> writes: > OK, I guess the commit message should be improved a bit. How about > this? > Hi Andreas, The description to the logic can go to comments, so that we don't need to do "git blame/log" to understand the code. > A field f in a structure composed of DWARF pieces may be located in > multiple pieces, where the first and last of those may contain bits > from other fields as well. So when writing to f, the beginning of the > first and the end of the last of those pieces may have to be skipped. > But the logic in write_pieced_value for handling one of those pieces > is flawed when the first and last piece are the same, i.e., f is > contained in a single piece: > > < - - - - - - - - - piece_size - - - - - - - - - -> > +-------------------------------------------------+ > | skipped_bits | f_bits | / / / / / / / / / / | > +-------------------------------------------------+ > > The current logic determines the size of the sub-piece to operate on > by limiting the piece size to the bit size of f and then subtracting > the skipped bits: > > max (piece_size, f_bits) - skipped_bits > > Instead of: > > max (piece_size - skipped_bits, f_bits) > Given this example, the result is the same, which is "piece_size - skipped_bits", am I missing something? > So the resulting sub-piece size is corrupted, leading to wrong > handling of this piece in write_pieced_value. > >> >>> logic in write_pieced_value for handling this is flawed when there are >>> actually bits to skip at the beginning of the first piece: it truncates >>> the piece size towards the end *before* accounting for the skipped bits >>> at the beginning instead of the other way around. >>> >>> Note that the same bug was already found in read_pieced_value and fixed >>> there (but not in write_pieced_value), see PR 15391. >> >> Can we share the code in write_pieced_value and read_pieced_value? The >> code computing offsets and bits should be shared. > > Yes. I have another patch (not posted yet) that merges these two > functions. I moved that towards the end of the patch series, so the > individual fixes can be incremental. > I'd like to merge the code first, then don't need to fix the same problem in two functions read_pieced_value and write_pieced_value (your patch 4/9 ~ 9/9 touches both two functions). >> Also, we need more comments in the code to explain these offsets and >> bits, a diagram about the relationships of these bits and offsets is >> quite helpful. > > OK. Some of the offset variables are removed by my patches, so I guess > I'll postpone that to the merged version. I'll see what I can come up > with and include it in v2. Please include it in V2.
On Wed, Apr 19 2017, Yao Qi wrote: > Andreas Arnez <arnez@linux.vnet.ibm.com> writes: > >> OK, I guess the commit message should be improved a bit. How about >> this? >> > > Hi Andreas, > The description to the logic can go to comments, so that we don't need > to do "git blame/log" to understand the code. Right, I'll add some general explanation and a diagram about the various bits and offsets (as requested below). However, most of the commit message explains a specific bug in a piece of code that won't exist any more. This aspect doesn't make sense to be included in the comments, I think. > >> A field f in a structure composed of DWARF pieces may be located in >> multiple pieces, where the first and last of those may contain bits >> from other fields as well. So when writing to f, the beginning of the >> first and the end of the last of those pieces may have to be skipped. >> But the logic in write_pieced_value for handling one of those pieces >> is flawed when the first and last piece are the same, i.e., f is >> contained in a single piece: >> >> < - - - - - - - - - piece_size - - - - - - - - - -> >> +-------------------------------------------------+ >> | skipped_bits | f_bits | / / / / / / / / / / | >> +-------------------------------------------------+ >> >> The current logic determines the size of the sub-piece to operate on >> by limiting the piece size to the bit size of f and then subtracting >> the skipped bits: >> >> max (piece_size, f_bits) - skipped_bits >> >> Instead of: >> >> max (piece_size - skipped_bits, f_bits) >> > > Given this example, the result is the same, which is > "piece_size - skipped_bits", am I missing something? Argh, the "max" above is obviously meant to be "min". Sorry for the confusion. > >> So the resulting sub-piece size is corrupted, leading to wrong >> handling of this piece in write_pieced_value. >> >>> >>>> logic in write_pieced_value for handling this is flawed when there are >>>> actually bits to skip at the beginning of the first piece: it truncates >>>> the piece size towards the end *before* accounting for the skipped bits >>>> at the beginning instead of the other way around. >>>> >>>> Note that the same bug was already found in read_pieced_value and fixed >>>> there (but not in write_pieced_value), see PR 15391. >>> >>> Can we share the code in write_pieced_value and read_pieced_value? The >>> code computing offsets and bits should be shared. >> >> Yes. I have another patch (not posted yet) that merges these two >> functions. I moved that towards the end of the patch series, so the >> individual fixes can be incremental. >> > > I'd like to merge the code first, then don't need to fix the same > problem in two functions read_pieced_value and write_pieced_value (your > patch 4/9 ~ 9/9 touches both two functions). Not sure I understand. Do you mean to merge the functions first while preserving existing logic, including all the bugs and differences? I had started along this path and gave up on it, because I found it too complicated. From that attempt I've concluded that the current approach is much less error-prone and easier to follow. > >>> Also, we need more comments in the code to explain these offsets and >>> bits, a diagram about the relationships of these bits and offsets is >>> quite helpful. >> >> OK. Some of the offset variables are removed by my patches, so I guess >> I'll postpone that to the merged version. I'll see what I can come up >> with and include it in v2. > > Please include it in V2. Sure. -- Andreas
Andreas Arnez <arnez@linux.vnet.ibm.com> writes: >> Hi Andreas, >> The description to the logic can go to comments, so that we don't need >> to do "git blame/log" to understand the code. > > Right, I'll add some general explanation and a diagram about the various > bits and offsets (as requested below). > > However, most of the commit message explains a specific bug in a piece > of code that won't exist any more. This aspect doesn't make sense to be > included in the comments, I think. > OK, no problem. >>>>> logic in write_pieced_value for handling this is flawed when there are >>>>> actually bits to skip at the beginning of the first piece: it truncates >>>>> the piece size towards the end *before* accounting for the skipped bits >>>>> at the beginning instead of the other way around. >>>>> >>>>> Note that the same bug was already found in read_pieced_value and fixed >>>>> there (but not in write_pieced_value), see PR 15391. >>>> >>>> Can we share the code in write_pieced_value and read_pieced_value? The >>>> code computing offsets and bits should be shared. >>> >>> Yes. I have another patch (not posted yet) that merges these two >>> functions. I moved that towards the end of the patch series, so the >>> individual fixes can be incremental. >>> >> >> I'd like to merge the code first, then don't need to fix the same >> problem in two functions read_pieced_value and write_pieced_value (your >> patch 4/9 ~ 9/9 touches both two functions). > > Not sure I understand. Do you mean to merge the functions first while > preserving existing logic, including all the bugs and differences? I What I meant is that 1) make two parts identical (but not introducing new bugs) 2) merge the code to a single function, 3) fix the rest of bugs in the single function, > had started along this path and gave up on it, because I found it too > complicated. From that attempt I've concluded that the current approach > is much less error-prone and easier to follow. There may be some complexity I didn't realize. I don't want to make your life harder. If you are comfortable on this approach, fine by me.
diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c index 93c45a7..496400a 100644 --- a/gdb/dwarf2loc.c +++ b/gdb/dwarf2loc.c @@ -1963,8 +1963,6 @@ write_pieced_value (struct value *to, struct value *from) bits_to_skip -= this_size_bits; continue; } - if (this_size_bits > type_len - offset) - this_size_bits = type_len - offset; if (bits_to_skip > 0) { dest_offset_bits = bits_to_skip; @@ -1977,6 +1975,8 @@ write_pieced_value (struct value *to, struct value *from) dest_offset_bits = 0; source_offset_bits = offset; } + if (this_size_bits > type_len - offset) + this_size_bits = type_len - offset; this_size = (this_size_bits + source_offset_bits % 8 + 7) / 8; source_offset = source_offset_bits / 8; diff --git a/gdb/testsuite/gdb.dwarf2/var-access.exp b/gdb/testsuite/gdb.dwarf2/var-access.exp index ee93b93..56a635a 100644 --- a/gdb/testsuite/gdb.dwarf2/var-access.exp +++ b/gdb/testsuite/gdb.dwarf2/var-access.exp @@ -174,6 +174,11 @@ gdb_test "print/d s1" " = \\{a = 63, b = 3, c = 0, d = 1\\}" \ "verify s1.a" gdb_test "print/d a" " = \\{0, 1, 63, 3, 4, 5, 6, 7\\}" \ "verify s1.a through a" +gdb_test_no_output "set var s1.b = 42" +gdb_test "print/d s1" " = \\{a = 63, b = 42, c = 0, d = 1\\}" \ + "verify s1.b" +gdb_test "print/d a" " = \\{0, 1, 63, 42, 4, 5, 6, 7\\}" \ + "verify s1.b through a" # Byte-aligned register- and memory pieces. gdb_test_no_output "set var \$[lindex $regname 0] = 81" \