This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
Thanks for the patch! > gold/ChangeLog: > > Egor Kochetov <egor.kochetov@intel.com> > * ehframe.h: updated function prototypes to enable discarding > FDEs with 0 PC range > * ehframe.cc: added filtering of FDEs to discard those with 0 PC range. The ChangeLog entry needs more detail -- there should be a separate item for each function modified. + int fde_pc_encoding; This is uninitialized, and may not be initialized in read_cie() if there is no 'R' augmentation. No matter, because it's actually unnecessary.... if (!this->read_cie(object, shndx, symbols, symbols_size, symbol_names, symbol_names_size, pcontents, p, pentend, &relocs, &cies, - new_cies)) + new_cies, fde_pc_encoding)) return false; } else { // FDE. if (!this->read_fde(object, shndx, symbols, symbols_size, - pcontents, id, p, pentend, &relocs, &cies)) + pcontents, id, p, pentend, &relocs, &cies, + fde_pc_encoding)) Indentation is off. Saving the encoding when reading the CIE, and using it when reading the next FDE is not only unnecessary, but wrong. Each FDE begins with a pointer to the CIE that it is based on. (While the x86 psABI says that the CIE pointer points the the "nearest preceding CIE", neither the LSB nor the DWARF spec for call frame information say that, so it is dangerous to assume that an FDE will always refer to the last CIE we saw. I hope the x86 psABI doesn't actually mean that, because that would make the CIE optimizations we do invalid.) - New_cies* new_cies) + New_cies* new_cies, + int& fde_pc_encoding) This should be a pointer, not a reference. We only use reference parameters when they can be const. - switch (fde_encoding & 7) + switch ((fde_pc_encoding = fde_encoding & 7)) I'd prefer not to embed assignments in the middle of a switch expression. This objection is moot, though, since we don't need to save it here. + switch (fde_pc_encoding) { Here, we can use the FDE encoding stored in the CIE, which we already have a pointer to. + case elfcpp::DW_EH_PE_udata2: is_zero_range = *(uint16_t*)(pfde + 2) == 0; break; + case elfcpp::DW_EH_PE_udata4: is_zero_range = *(uint32_t*)(pfde + 4) == 0; break; + case elfcpp::DW_EH_PE_udata8: is_zero_range = *(uint64_t*)(pfde + 8) == 0; break; These lines are too long. In gold, we use static_cast<>() instead of old-style casts. Since we're only testing for zero here, we don't strictly need to honor the byte order, but I'd still prefer to use elfcpp::Swap::readval() for consistency and safety (e.g., in case someone later modifies this code or copies it somewhere else where we're not just testing for 0). + default: + // All other cases were rejected in Eh_frame::read_cie. + gold_unreachable(); Actually, read_cie() doesn't reject DW_EH_PE_absptr, which you haven't handled here. + if (is_zero_range + || (is_ordinary && fde_shndx != elfcpp::SHN_UNDEF && fde_shndx < object->shnum() - && !object->is_section_included(fde_shndx)) + && !object->is_section_included(fde_shndx))) Indentation needs adjusting. I've committed the attached patch. -cary 2016-02-26 Egor Kochetov <egor.kochetov@intel.com> Cary Coutant <ccoutant@gmail.com> PR gold/19735 * ehframe.h (Cie::fde_encoding): New method. * ehframe.cc (Eh_frame::read_fde): Discard FDEs for zero-length address ranges.
Attachment:
pr19735.patch
Description: Binary data
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |