On Thu, Mar 25, 2021 at 2:38 PM Gedare Bloom <ged...@rtems.org> wrote: > > On Thu, Mar 25, 2021 at 10:05 AM Joel Sherrill <j...@rtems.org> wrote: > > > > > > > > On Thu, Mar 25, 2021 at 10:42 AM Gedare Bloom <ged...@rtems.org> wrote: > >> > >> On Thu, Mar 18, 2021 at 12:05 PM Alex White <alex.wh...@oarcorp.com> wrote: > >> > - // Mark the start of each instruction in the coverage map. > >> > - for (auto& instruction : instructions) { > >> > - coverageMap.setIsStartOfInstruction( instruction.address ); > >> > - } > >> > + // Find the address of the first instruction. > >> > + for (auto& line : instructions) { > >> > + if (!line.isInstruction) { > >> > + continue; > >> > + } > >> > + > >> > + firstInstructionAddress = line.address; > >> > + break; > >> > + } > >> This is a poorly written loop. Anytime you use continue/break to > >> control the loop iteration, something is going awry. You can write > >> this code equivalently without any continue or break. > > > > > > This loop is very deliberately constructed not to be a for loop because > > you need the ability to break at different points to debug issues. Alex > > and I spent hours in this loop debugging which I think would have > > been more difficult to debug if all the conditions were in a for loop. > > This loop was written to explicitly have breakpoints for various > > conditions. > > > > There have been multiple issues with the information reported with > > size/length being at the top of that list. Before Alex found the source > > of that (not covoar), it resulted in an infinite loop. > > > > I suppose it can be changed to a for loop as purer academically but > > it loses significantly on ability to set breakpoints and debug if there > > ever is an issue again. > > > > Are you willing to make that trade? > > > > You know that what we debug is often much uglier than what we publish. > At least, simplify the logic to let the loop work for you. > > for (auto& line : instructions) { > if ( line.isInstruction ) { > firstInstructionAddress = line.address; > break; > } > }
I will make this change. > > Using continue/break to control loops is antithetical to using loops, > and probably doesn't do the compiler any good. > > >> > + if (!line.isInstruction) { > >> > + continue; > >> > + } > >> > + > >> > + > >> > + break; > >> > + } > >> > >> > + > >> > + if (firstInstructionAddress == UINT32_MAX) { > >> > + std::ostringstream what; > >> > + what << "Could not find first instruction address for symbol " > >> > + << symbolName << " in " << executableInfo->getFileName(); > >> > + throw rld::error( what, "Coverage::finalizeSymbol" ); > >> > + } > >> > + > >> > + int rangeIndex; > >> > + uint32_t lowAddress = UINT32_MAX; > >> > + for (rangeIndex = 0; ; rangeIndex++) { > >> > + lowAddress = coverageMap.getLowAddressOfRange(rangeIndex); > >> > + > >> > + if (firstInstructionAddress == lowAddress) { > >> > + break; > >> ditto... if you're goin to break the loop on this condition, why not > >> use this in your for loop? > >> for (rangeIndex = 0; firstInstructionAddress != lowAddress ; rangeIndex++) > >> { I will make this change, too. > >> > >> > + } > >> > + } > >> > > >> > - // Create a unified coverage map for the symbol. > >> > - SymbolsToAnalyze->createCoverageMap( > >> > - executableInfo->getFileName().c_str(), symbolName, size > >> > - ); > >> > + uint32_t sizeWithoutNops = coverageMap.getSizeOfRange(rangeIndex); > >> > + uint32_t size = sizeWithoutNops; > >> > + uint32_t highAddress = lowAddress + size - 1; > >> > + uint32_t computedHighAddress = highAddress; > >> > + > >> > + // Find the high address as reported by the address of the last > >> > NOP > >> > + // instruction. This ensures that NOPs get marked as executed > >> > later. > >> > + for (auto instruction = instructions.rbegin(); > >> > + instruction != instructions.rend(); > >> > + instruction++) { > >> > + if (instruction->isInstruction) { > >> > + if (instruction->isNop) { > >> > + computedHighAddress = instruction->address + > >> > instruction->nopSize; > >> > + } > >> > + break; > >> Here too. I'm not sure I see a more clear way to write this loop while eliminating the `break` statement. Do you have a suggestion? > >> > >> > + } > >> > + } > >> > + > >> > + if (highAddress != computedHighAddress) { > >> > + std::cerr << "Function's high address differs between DWARF and > >> > objdump: " > >> > + << symbolName << " (0x" << std::hex << highAddress << " and > >> > 0x" > >> > + << computedHighAddress - 1 << ")" << std::dec << std::endl; > >> > + size = computedHighAddress - lowAddress; > >> > + } > >> > + > >> > + // If there are NOT already saved instructions, save them. > >> > + SymbolInformation* symbolInfo = SymbolsToAnalyze->find( > >> > symbolName ); > >> > + if (symbolInfo->instructions.empty()) { > >> > + symbolInfo->sourceFile = executableInfo; > >> > + symbolInfo->baseAddress = lowAddress; > >> > + symbolInfo->instructions = instructions; > >> > + } > >> > + > >> > + // Add the symbol to this executable's symbol table. > >> > + SymbolTable* theSymbolTable = executableInfo->getSymbolTable(); > >> > + theSymbolTable->addSymbol( > >> > + symbolName, lowAddress, highAddress - lowAddress + 1 > >> > + ); > >> > + > >> > + // Mark the start of each instruction in the coverage map. > >> > + for (auto& instruction : instructions) { > >> > + coverageMap.setIsStartOfInstruction( instruction.address ); > >> > + } > >> > + > >> > + // Create a unified coverage map for the symbol. > >> > + SymbolsToAnalyze->createCoverageMap( > >> > + executableInfo->getFileName().c_str(), symbolName, size, > >> > sizeWithoutNops > >> > + ); > >> > + } catch (const ExecutableInfo::CoverageMapNotFoundError& e) { > >> > + // Allow execution to continue even if a coverage map could not be > >> > + // found. > >> > + std::cerr << "Coverage map not found for symbol " << e.what() > >> > + << std::endl; > >> > + } > >> > } > >> > > >> > ObjdumpProcessor::ObjdumpProcessor() > >> > -- > >> > 2.27.0 > >> > > >> > _______________________________________________ > >> > devel mailing list > >> > devel@rtems.org > >> > http://lists.rtems.org/mailman/listinfo/devel > >> _______________________________________________ > >> devel mailing list > >> devel@rtems.org > >> http://lists.rtems.org/mailman/listinfo/devel > _______________________________________________ > devel mailing list > devel@rtems.org > http://lists.rtems.org/mailman/listinfo/devel _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel