I'm looking at 6200ba6.
* Cosmetic trivia: casts should look like "(type) expression". When I either type is void * (like d_buf), don't cast at all.
* What is read_length for? I don't think there can be any relocs in the DW_FORM_block* length fields. What other use do you envision for it?
* read_offset should take another IDX_* argument for the target section into which the offset points. (Sorry, I thought I had said that before.) This lets us consolidate the basic offset sanity checking there.
* s/Dwarf_Addr/Dwarf_Off/ in the read_offset signature.
* DW_FORM_ref[48] should not have relocs. Those values are CU-relative and must always be computed by arithmetic at assembly time. Of the reference forms, I think only DW_FORM_ref_addr can have a reloc.
* In both pubnames and formref_die, s/address/offset/.
* In dwarf_getlocation_addr, s/IDX_debug_info/IDX_debug_loc/.
* Make __libdw_in_section return bool. It only makes sense for helpers to return int (0/-1) when that might be usable directly as the return value by a public dwarf_* caller.
* In dwarf_getpubnames, only the cu_offset header field can have a reloc. The die_offset fields are CU-relative.
* There are two cases of reading a begin/end pair of address_size words: in location lists (dwarf_getlocation.c), and in range lists (dwarf_ranges.c). These should be consolidated into a single helper that we can make reloc-savvy (might as well put it in dwarf_ranges.c). That can do these several common things:
* Take the base-address logic from dwarf_getlocation.c (start with -1 and initialize it on demand). * A base address entry should be one where there is no reloc on the "begin" word, so it is literal -1. * An end of list entry should be one where there are no relocs on either word and both are literal 0.
We don't need any new logic for the moment, just make it a stub for now and we can fill it in with the real reloc support when we do that to the other stubs.
* dwarf_formudata needs special treatment.
If DW_FORM_data[48] have relocs for a "constant", those are the read_address flavor, not the read_offset flavor. Really "address" doesn't mean "address", it means "relocated with reference to allocated sections". So if it has any relocs at all, they will be that flavor.
For the internal calls for the "class *ptr" attributes, we should use a new internal helper in place of dwarf_formudata. That helper can take the attribute, the target-section IDX_*, and a Dwarf_Off * to fill in. It does the form decode and read_offset together.
In fact, looking at all the read_offset uses, I think we should roll a bit more in there. Instead of yielding a Dwarf_Off, it can just yield a const unsigned char *. It should take an additional argument that is the DWARF_E_* code to fail with if the target section is missing. It might be simplest to make it also just yield an endp, to we can make everything use endp - readp < n comparisons instead of d_size checks.
Thanks, Roland
Roland McGrath wrote:
- What is read_length for? I don't think there can be any relocs in the DW_FORM_block* length fields. What other use do you envision for it?
Probably nothing. I rolled it in when I've hit some length-related code, having remembered that you've mentioned length in one of the how-to emails that you had posted earlier. I was mostly expecting it would turn out being useless, but left it for cross-checking with dwarflint. I dropped it now.
- In dwarf_getlocation_addr, s/IDX_debug_info/IDX_debug_loc/.
Do you mean s/IDX_debug_line/IDX_debug_loc/? That's what I've done. I think the debug_info is correct, but debug_line seem to be erroneous.
- There are two cases of reading a begin/end pair of address_size words
I created __libdw_read_begin_end_pair_inc with common bits of begin/end handling. It doesn't use __libdw_read_address but instead uses READ_AND_RELOCATE macro directly, so that it knows whether the relocation was present or not (the macro was changed to provide that information to the caller). Or is it necessary to have reloc processing directly in that method? The way it's written now, it uses the same reloc hook as __libdw_read_address.
- dwarf_formudata needs special treatment.
Will hopefully do later today.
The rest that I snipped is done according to your comments.
Thanks, PM
Roland McGrath wrote:
- What is read_length for? I don't think there can be any relocs in the DW_FORM_block* length fields. What other use do you envision for it?
Probably nothing. I rolled it in when I've hit some length-related code, having remembered that you've mentioned length in one of the how-to emails that you had posted earlier. I was mostly expecting it would turn out being useless, but left it for cross-checking with dwarflint. I dropped it now.
Ok. It doesn't hurt to have wrappers that indicate semantics like that when they compile away to nothing, if it makes the code easier to understand. I just wanted to make sure we're clear on the (non)expectation of relocs there.
- In dwarf_getlocation_addr, s/IDX_debug_info/IDX_debug_loc/.
Do you mean s/IDX_debug_line/IDX_debug_loc/? That's what I've done. I think the debug_info is correct, but debug_line seem to be erroneous.
Yes, sorry.
- There are two cases of reading a begin/end pair of address_size words
I created __libdw_read_begin_end_pair_inc with common bits of begin/end handling. It doesn't use __libdw_read_address but instead uses READ_AND_RELOCATE macro directly, so that it knows whether the relocation was present or not (the macro was changed to provide that information to the caller).
I haven't reviewed that in detail yet, but it looks good off hand.
Or is it necessary to have reloc processing directly in that method? The way it's written now, it uses the same reloc hook as __libdw_read_address.
That's as it should be.
Thanks, Roland
Roland McGrath wrote:
- dwarf_formudata needs special treatment.
This is now on the branch. The new function is called __libdw_read_udata_address. There's a mismatch between what the function does and what the code in dwarf_ranges looks like, I need to look that some more tomorrow.
Doing the above, I found myself looking at dwarf_getmacros. The way offsets are handled in this functions strikes me as wrong: macoff is read from attribute, and then never used; instead, readp is formed as d_buf + offset, where offset is an argument passed into the function (seems to be for purposes of iteration restarts). readendp is then computed as readp + d_size, which is definitely wrong. Our test suite doesn't cover this function (AFAIK, GNU toolchain doesn't even emit macinfo section). Seems like something that should be fixed. I can look at it tomorrow, on separate branch. Right now I'm leaving it alone.
Thanks, PM
Roland McGrath wrote:
- dwarf_formudata needs special treatment.
This is now on the branch. The new function is called __libdw_read_udata_address.
"address" does not make sense here--it's an offset that's being relocated. It should use the read_offset hook internally. I would call it something like __libdw_formptr, since it's the attribute decoder for "*ptr" classes.
It should not reject the other {u,}data* forms that can't have relocs. It's not mandatory that relocatable forms be used in a final-link section. (Our writer will probably use the shortest form in this case.)
I think the bogus offset check case should use DWARF_E_INVALID_DWARF, not the ERR_NODATA error. Use an unlikely () in any if that leads to a malformed-data sort of error.
Doing the above, I found myself looking at dwarf_getmacros. The way offsets are handled in this functions strikes me as wrong: macoff is read from attribute, and then never used; [...]
Yes, it's wrong. It was never noticed since this was so little used or tested, and it always happens to work for the first CU in the section so that trivial test cases don't hit the bug. A test case and fix for that for the trunk would be good.
(AFAIK, GNU toolchain doesn't even emit macinfo section).
Use -g3 to request it.
Thanks, Roland
Roland McGrath wrote:
Roland McGrath wrote:
- dwarf_formudata needs special treatment.
This is now on the branch. The new function is called __libdw_read_udata_address.
"address" does not make sense here--it's an offset that's being relocated. It should use the read_offset hook internally. I would call it something like __libdw_formptr, since it's the attribute decoder for "*ptr" classes.
Aha, I believe I misunderstood what you said earlier. I _moved_ address relocation semantics into formptr, instead of leaving them in formudata and introducing offset semantics in formptr. Fixed now.
The rest is in, too.
Thanks, PM
I did a quick fresh review of 8b1aad2.
Two cosmetic things still need fixed: Use a space in cast syntax. To refer to local vars in comments, /* Here FOO is used. */ rather than /* Here `foo' is used. */
Also, in libdwP.h non-defining decls, put the function name on the same line as the type. Three cosmetic things.
__libdw_read_begin_end_pair_inc inverts the return value check on READ_AND_RELOCATE, no? Since that's the only place that looks at the value of the expression, just make its value a "was relocated" Boolean (i.e. "status > 1").
I don't understand the reason for the uintptr_t cast in READ_AND_RELOCATE. The hooks can just take a void * argument, no need for any casts.
I think we can just drop libdw_readhooks.c entirely for now. Either make the two no-op stubs inlines in libdwP.h, or just tweak READ_AND_RELOCATE to comment out the use of RELOC_HOOK. (The inlines are better if there are any callers' arguments or locals that are unused without the RELOC_HOOK call.) I guess you'll want stubs there to do the testing anyhow, the inlines should do fine. (When the real hooks go in, they will be in libdw/relocate.c along with the innards of the reloc-aware logic. Before that we want to merge onto the trunk with stub inlines that compile away.)
dwarf_formstring could use __libdw_formptr.
dwarf_getaranges and dwarf_getpubnames both had sanity checks on the offsets like "offset + 3 > d_size" (or 4). Without these, a bogus offset could make them read off the end of the buffer in their fixed-size header reading bit. Make read_offset take a "minimum to read" argument (pass 0 or 1 in the other places), so the consolidated check for bogus offsets can calculate that in. Be sure those checks are immune from integer/pointer arithmetic overflow from bogus offsets, like: if (unlikely (offset > d_size) || unlikely (d_size - offset < minread))
These final nits are small and should be quick to take care of. It's excellent work all together. Great job! Are you about ready to move on to the testing regime?
Thanks, Roland
Roland McGrath wrote:
__libdw_read_begin_end_pair_inc inverts the return value check on READ_AND_RELOCATE, no? Since that's the only place that looks at the value of the expression, just make its value a "was relocated" Boolean (i.e. "status > 1").
Right, there was a bug there.
I think we can just drop libdw_readhooks.c entirely for now. Either make the two no-op stubs inlines in libdwP.h
Done.
dwarf_formstring could use __libdw_formptr.
Well that gets rid of a bit of code in dwarf_formstring, but then __libdw_formptr has to handle two distinct cases: DW_FORM_strp, and DW_FORM_data* (when one is valid the other is not). I just don't think it's worth it to add that kind of flexibility just for sake of one case.
dwarf_getaranges and dwarf_getpubnames both had sanity checks on the offsets like "offset + 3 > d_size" (or 4). Without these, a bogus offset could make them read off the end of the buffer in their fixed-size header reading bit. Make read_offset take a "minimum to read" argument (pass 0 or 1 in the other places), so the consolidated check for bogus offsets can calculate that in. Be sure those checks are immune from integer/pointer arithmetic overflow from bogus offsets, like: if (unlikely (offset > d_size) || unlikely (d_size - offset < minread))
I've rewritten the boundary checking logic to be like that.
Are you about ready to move on to the testing regime?
I'm ready when you are.
PM
Well that gets rid of a bit of code in dwarf_formstring, but then __libdw_formptr has to handle two distinct cases: DW_FORM_strp, and DW_FORM_data* (when one is valid the other is not). I just don't think it's worth it to add that kind of flexibility just for sake of one case.
Good point.
Are you about ready to move on to the testing regime?
I'm ready when you are.
Ok! Am I ready? :-) What kind of ready should I be? Is the plan I explained clear? (Did I explain it?) The first stage is to do the testing hacks and verify on the build's own .o files. After that the debuginfo test archive 'rel' set. I had the impression you had a debuginfo archive set up yourself now. What do you need me to test?
Thanks, Roland
Roland McGrath wrote:
Are you about ready to move on to the testing regime?
I'm ready when you are.
Ok! Am I ready? :-) What kind of ready should I be? Is the plan I explained clear? (Did I explain it?) The first stage is to do the testing hacks and verify on the build's own .o files. After that the debuginfo test archive 'rel' set. I had the impression you had a debuginfo archive set up yourself now. What do you need me to test?
You explained it well enough for me to get on with the testing. I though you have something more on mind, is why I used that phrase. If not, I'll proceed.
PM
Roland McGrath wrote:
You explained it well enough for me to get on with the testing. I though you have something more on mind, is why I used that phrase. If not, I'll proceed.
In mind? Me? In what mind?!?
Hard to say, since you seem to be out of the same last two messages :)
PM
Doing the above, I found myself looking at dwarf_getmacros. The way offsets are handled in this functions strikes me as wrong: macoff is read from attribute, and then never used; [...]
Yes, it's wrong. It was never noticed since this was so little used or tested, and it always happens to work for the first CU in the section so that trivial test cases don't hit the bug. A test case and fix for that for the trunk would be good.
pmachata/pending has a fix and a test case. Please review.
PM
pmachata/pending has a fix and a test case. Please review.
I merged it, thanks. Please note the trivial change I committed after. Part is purely cosmetic (ChangeLog syntax, avoid line breaks in a () list). The Makefile.am change is partly cosmetic (keep the scripts together in the list and testfiles at the end), and also that you omitted the new test file. (You can always use a 'make -s distcheck' to check for any trivial omissions like that.)
Thanks, Roland
elfutils-devel@lists.fedorahosted.org