On Thu, Mar 25, 2021 at 2:42 PM Alex White <alex.wh...@oarcorp.com> wrote:
>
> 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?
>
no, this one is alright, since you have a special action to take on
the last iteration.
fwiw, the isInstruction and isNop ought to be methods and not directly
accessing boolean variables, but that is just another C'ism nit ;)
> > >> > +
> > >> > + 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
