Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PPCSymbolDB: Fix getting symbol for the last function #10924

Merged

Conversation

Pokechu22
Copy link
Contributor

It's easiest to illustrate the bug that I fixed with an example from System Menu 4.3U (the functions I'm using here are arbitrary, chosen just because they're short):

Step Before After
Set address to 815344f0 image image
Add function at 815344f4 image image
Add function at 8153450c image image
Add function at 81534548 image image

(In the debugger, to set the address, use the top-left text field labeled search address, and to add a function, right-click the specified address and select "add function" (2nd entry in the bottom group, below "run to here").)

This is an issue I ran into on #10923, and doesn't just affect "add function".

@dolphin-emu-bot
Copy link
Contributor

FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:

  • ab11-homebrew on ogl-lin-radeon: diff
  • aeon-charge-attack on ogl-lin-radeon: diff
  • bk-tev on ogl-lin-radeon: diff
  • burnout2-vehicletextures on ogl-lin-radeon: diff
  • chibi-robo-fastdepth on ogl-lin-radeon: diff
  • chibi-robo-zfighting on ogl-lin-radeon: diff
  • custom-brawl-char on ogl-lin-radeon: diff
  • dbz-depth on ogl-lin-radeon: diff
  • djfny-menu on ogl-lin-radeon: diff
  • djhero2-blend on ogl-lin-radeon: diff
  • DKCR-Char on ogl-lin-radeon: diff
  • DKCR-fast-depth on ogl-lin-radeon: diff
  • ea-pink on ogl-lin-radeon: diff
  • ea-vp6 on ogl-lin-radeon: diff
  • ed-updated on ogl-lin-radeon: diff
  • et-vid on ogl-lin-radeon: diff
  • find-mii on ogl-lin-radeon: diff
  • fishing-resort-map on ogl-lin-radeon: diff
  • fog-adj on ogl-lin-radeon: diff
  • fortune-street on ogl-lin-radeon: diff
  • fortune-street-fog on ogl-lin-radeon: diff
  • fortune-street-white-box on ogl-lin-radeon: diff
  • fsa-layers on ogl-lin-radeon: diff
  • f-zero-rain on ogl-lin-radeon: diff
  • goldeneye-depth on ogl-lin-radeon: diff
  • hb-discgolf on ogl-lin-radeon: diff
  • inverted-depth-range on ogl-lin-radeon: diff
  • jb-shadow on ogl-lin-radeon: diff
  • jd2-fmv on ogl-lin-radeon: diff
  • jj-awae-mirrored on ogl-lin-radeon: diff
  • kirby-logicop on ogl-lin-radeon: diff
  • kirby-shadows on ogl-lin-radeon: diff
  • last-story-shadows on ogl-lin-radeon: diff
  • lego-star-wars-crane-shadow on ogl-lin-radeon: diff
  • lesson08 on ogl-lin-radeon: diff
  • line-width-test on ogl-lin-radeon: diff
  • lm-mario-portrait on ogl-lin-radeon: diff
  • luigi-shadows on ogl-lin-radeon: diff
  • major-minor on ogl-lin-radeon: diff
  • mario-baseball-shadows on ogl-lin-radeon: diff
  • mario-golf-oob on ogl-lin-radeon: diff
  • mario-sluggers-bar on ogl-lin-radeon: diff
  • mario-tennis-menu on ogl-lin-radeon: diff
  • MaS-LOG-wiimote on ogl-lin-radeon: diff
  • megaman-heat on ogl-lin-radeon: diff
  • melee-depth on ogl-lin-radeon: diff
  • melee-lighting on ogl-lin-radeon: diff
  • metroid-visor on ogl-lin-radeon: diff
  • mii-channel on ogl-lin-radeon: diff
  • milotic-texture on ogl-lin-radeon: diff
  • mini-ninjas on ogl-lin-radeon: diff
  • mkdd-babypark on ogl-lin-radeon: diff
  • mkdd-efb on ogl-lin-radeon: diff
  • mkw-bridge on ogl-lin-radeon: diff
  • mkw-flags on ogl-lin-radeon: diff
  • mkwii-bluebox on ogl-lin-radeon: diff
  • mmx-light on ogl-lin-radeon: diff
  • monkeyball-fuse on ogl-lin-radeon: diff
  • mp2-scanner on ogl-lin-radeon: diff
  • mp3-bloom on ogl-lin-radeon: diff
  • mp4-vertexcache on ogl-lin-radeon: diff
  • mp7-text on ogl-lin-radeon: diff
  • mp8-widescreen on ogl-lin-radeon: diff
  • mtennis-zfreeze on ogl-lin-radeon: diff
  • my-word-coach on ogl-lin-radeon: diff
  • nddemo-bumpmapping on ogl-lin-radeon: diff
  • nddemo-lighting on ogl-lin-radeon: diff
  • nes-vc on ogl-lin-radeon: diff
  • nfsu-purplerect on ogl-lin-radeon: diff
  • nfsu-reflections on ogl-lin-radeon: diff
  • nhl-slap on ogl-lin-radeon: diff
  • nintendo-channel on ogl-lin-radeon: diff
  • nsmbw-coins on ogl-lin-radeon: diff
  • nsmbw-intro on ogl-lin-radeon: diff
  • pbr-sfx on ogl-lin-radeon: diff
  • pm-hc-jp on ogl-lin-radeon: diff
  • puzzle-collection on ogl-lin-radeon: diff
  • pw-black-bars on ogl-lin-radeon: diff
  • quake-gx on ogl-lin-radeon: diff
  • rs2-bumpmapping on ogl-lin-radeon: diff
  • rs2-glass on ogl-lin-radeon: diff
  • rs2-skybox on ogl-lin-radeon: diff
  • rs2-zfreeze on ogl-lin-radeon: diff
  • rs3-bumpmapping on ogl-lin-radeon: diff
  • rs3-skybox2 on ogl-lin-radeon: diff
  • sadx-ui on ogl-lin-radeon: diff
  • sfa-shadows on ogl-lin-radeon: diff
  • sf-assault-flashing on ogl-lin-radeon: diff
  • shadow-eyes on ogl-lin-radeon: diff
  • simpsons-game on ogl-lin-radeon: diff
  • smb-mirror on ogl-lin-radeon: diff
  • smg2-fog on ogl-lin-radeon: diff
  • smg-marioeyes on ogl-lin-radeon: diff
  • smg-mmg on ogl-lin-radeon: diff
  • smg-roar on ogl-lin-radeon: diff
  • sms-bubbles on ogl-lin-radeon: diff
  • sms-gc on ogl-lin-radeon: diff
  • sms-water on ogl-lin-radeon: diff
  • soa-black on ogl-lin-radeon: diff
  • soniccolors-mm on ogl-lin-radeon: diff
  • sonic-riders-blur on ogl-lin-radeon: diff
  • sonic-riders-zg-4p on ogl-lin-radeon: diff
  • sonicriderszg-gb on ogl-lin-radeon: diff
  • spyro-bloom on ogl-lin-radeon: diff
  • spyro-depth on ogl-lin-radeon: diff
  • ssbb-mod-lloyd on ogl-lin-radeon: diff
  • ssbm-pointsize on ogl-lin-radeon: diff
  • ss-map on ogl-lin-radeon: diff
  • super-sluggers-white-out on ogl-lin-radeon: diff
  • sw3-dt on ogl-lin-radeon: diff
  • taiko-depth on ogl-lin-radeon: diff
  • thps3-earlyz on ogl-lin-radeon: diff
  • thps4-shadow on ogl-lin-radeon: diff
  • tla-menu on ogl-lin-radeon: diff
  • tos-invis-char on ogl-lin-radeon: diff
  • tp-skin on ogl-lin-radeon: diff
  • tsp3-pinkgrass on ogl-lin-radeon: diff
  • vegas-party-depth on ogl-lin-radeon: diff
  • viewitful-joe-distortion on ogl-lin-radeon: diff
  • xenoblade-menu on ogl-lin-radeon: diff
  • zelda1-vc on ogl-lin-radeon: diff
  • ztp-grass on ogl-lin-radeon: diff
  • zww-armos on ogl-lin-radeon: diff
  • zww-water on ogl-lin-radeon: diff
  • zww-waves on ogl-lin-radeon: diff
  • ab11-homebrew on uberogl-lin-radeon: diff
  • aeon-charge-attack on uberogl-lin-radeon: diff
  • bk-tev on uberogl-lin-radeon: diff
  • burnout2-vehicletextures on uberogl-lin-radeon: diff
  • chibi-robo-fastdepth on uberogl-lin-radeon: diff
  • chibi-robo-zfighting on uberogl-lin-radeon: diff
  • custom-brawl-char on uberogl-lin-radeon: diff
  • dbz-depth on uberogl-lin-radeon: diff
  • djfny-menu on uberogl-lin-radeon: diff
  • djhero2-blend on uberogl-lin-radeon: diff
  • DKCR-Char on uberogl-lin-radeon: diff
  • DKCR-fast-depth on uberogl-lin-radeon: diff
  • ea-pink on uberogl-lin-radeon: diff
  • ea-vp6 on uberogl-lin-radeon: diff
  • ed-updated on uberogl-lin-radeon: diff
  • et-vid on uberogl-lin-radeon: diff
  • find-mii on uberogl-lin-radeon: diff
  • fishing-resort-map on uberogl-lin-radeon: diff
  • fog-adj on uberogl-lin-radeon: diff
  • fortune-street on uberogl-lin-radeon: diff
  • fortune-street-fog on uberogl-lin-radeon: diff
  • fortune-street-white-box on uberogl-lin-radeon: diff
  • fsa-layers on uberogl-lin-radeon: diff
  • f-zero-rain on uberogl-lin-radeon: diff
  • goldeneye-depth on uberogl-lin-radeon: diff
  • hb-discgolf on uberogl-lin-radeon: diff
  • inverted-depth-range on uberogl-lin-radeon: diff
  • jb-shadow on uberogl-lin-radeon: diff
  • jd2-fmv on uberogl-lin-radeon: diff
  • jj-awae-mirrored on uberogl-lin-radeon: diff
  • kirby-logicop on uberogl-lin-radeon: diff
  • kirby-shadows on uberogl-lin-radeon: diff
  • last-story-shadows on uberogl-lin-radeon: diff
  • lego-star-wars-crane-shadow on uberogl-lin-radeon: diff
  • lesson08 on uberogl-lin-radeon: diff
  • line-width-test on uberogl-lin-radeon: diff
  • lm-mario-portrait on uberogl-lin-radeon: diff
  • luigi-shadows on uberogl-lin-radeon: diff
  • major-minor on uberogl-lin-radeon: diff
  • mario-baseball-shadows on uberogl-lin-radeon: diff
  • mario-golf-oob on uberogl-lin-radeon: diff
  • mario-sluggers-bar on uberogl-lin-radeon: diff
  • mario-tennis-menu on uberogl-lin-radeon: diff
  • MaS-LOG-wiimote on uberogl-lin-radeon: diff
  • megaman-heat on uberogl-lin-radeon: diff
  • melee-depth on uberogl-lin-radeon: diff
  • melee-lighting on uberogl-lin-radeon: diff
  • metroid-visor on uberogl-lin-radeon: diff
  • mii-channel on uberogl-lin-radeon: diff
  • milotic-texture on uberogl-lin-radeon: diff
  • mini-ninjas on uberogl-lin-radeon: diff
  • mkdd-babypark on uberogl-lin-radeon: diff
  • mkdd-efb on uberogl-lin-radeon: diff
  • mkw-bridge on uberogl-lin-radeon: diff
  • mkw-flags on uberogl-lin-radeon: diff
  • mkwii-bluebox on uberogl-lin-radeon: diff
  • mmx-light on uberogl-lin-radeon: diff
  • monkeyball-fuse on uberogl-lin-radeon: diff
  • mp2-scanner on uberogl-lin-radeon: diff
  • mp3-bloom on uberogl-lin-radeon: diff
  • mp4-vertexcache on uberogl-lin-radeon: diff
  • mp7-text on uberogl-lin-radeon: diff
  • mp8-widescreen on uberogl-lin-radeon: diff
  • mtennis-zfreeze on uberogl-lin-radeon: diff
  • my-word-coach on uberogl-lin-radeon: diff
  • nddemo-bumpmapping on uberogl-lin-radeon: diff
  • nddemo-lighting on uberogl-lin-radeon: diff
  • nes-vc on uberogl-lin-radeon: diff
  • nfsu-purplerect on uberogl-lin-radeon: diff
  • nfsu-reflections on uberogl-lin-radeon: diff
  • nhl-slap on uberogl-lin-radeon: diff
  • nintendo-channel on uberogl-lin-radeon: diff
  • nsmbw-coins on uberogl-lin-radeon: diff
  • nsmbw-intro on uberogl-lin-radeon: diff
  • pbr-sfx on uberogl-lin-radeon: diff
  • pm-hc-jp on uberogl-lin-radeon: diff
  • puzzle-collection on uberogl-lin-radeon: diff
  • pw-black-bars on uberogl-lin-radeon: diff
  • quake-gx on uberogl-lin-radeon: diff
  • rs2-bumpmapping on uberogl-lin-radeon: diff
  • rs2-glass on uberogl-lin-radeon: diff
  • rs2-skybox on uberogl-lin-radeon: diff
  • rs2-zfreeze on uberogl-lin-radeon: diff
  • rs3-bumpmapping on uberogl-lin-radeon: diff
  • rs3-skybox2 on uberogl-lin-radeon: diff
  • sadx-ui on uberogl-lin-radeon: diff
  • sfa-shadows on uberogl-lin-radeon: diff
  • sf-assault-flashing on uberogl-lin-radeon: diff
  • shadow-eyes on uberogl-lin-radeon: diff
  • simpsons-game on uberogl-lin-radeon: diff
  • smb-mirror on uberogl-lin-radeon: diff
  • smg2-fog on uberogl-lin-radeon: diff
  • smg-marioeyes on uberogl-lin-radeon: diff
  • smg-mmg on uberogl-lin-radeon: diff
  • smg-roar on uberogl-lin-radeon: diff
  • sms-bubbles on uberogl-lin-radeon: diff
  • sms-gc on uberogl-lin-radeon: diff
  • sms-water on uberogl-lin-radeon: diff
  • soa-black on uberogl-lin-radeon: diff
  • soniccolors-mm on uberogl-lin-radeon: diff
  • sonic-riders-blur on uberogl-lin-radeon: diff
  • sonic-riders-zg-4p on uberogl-lin-radeon: diff
  • sonicriderszg-gb on uberogl-lin-radeon: diff
  • spyro-bloom on uberogl-lin-radeon: diff
  • spyro-depth on uberogl-lin-radeon: diff
  • ssbb-mod-lloyd on uberogl-lin-radeon: diff
  • ssbm-pointsize on uberogl-lin-radeon: diff
  • ss-map on uberogl-lin-radeon: diff
  • super-sluggers-white-out on uberogl-lin-radeon: diff
  • sw3-dt on uberogl-lin-radeon: diff
  • taiko-depth on uberogl-lin-radeon: diff
  • thps3-earlyz on uberogl-lin-radeon: diff
  • thps4-shadow on uberogl-lin-radeon: diff
  • tla-menu on uberogl-lin-radeon: diff
  • tos-invis-char on uberogl-lin-radeon: diff
  • tp-skin on uberogl-lin-radeon: diff
  • tsp3-pinkgrass on uberogl-lin-radeon: diff
  • vegas-party-depth on uberogl-lin-radeon: diff
  • viewitful-joe-distortion on uberogl-lin-radeon: diff
  • xenoblade-menu on uberogl-lin-radeon: diff
  • zelda1-vc on uberogl-lin-radeon: diff
  • ztp-grass on uberogl-lin-radeon: diff
  • zww-armos on uberogl-lin-radeon: diff
  • zww-water on uberogl-lin-radeon: diff
  • zww-waves on uberogl-lin-radeon: diff

automated-fifoci-reporter

@AdmiralCurtiss
Copy link
Contributor

It took me a minute to understand why this works for the very first address: It hits the it != m_functions.end() && it->second.address == addr case, so ignoring it in the it != m_functions.begin() branch is fine.

@AdmiralCurtiss
Copy link
Contributor

AdmiralCurtiss commented Jul 30, 2022

I think this would be clearer as:

  if (it == m_functions.end())
    return nullptr;

  // If the address is exactly the start address of a symbol, we're done.
  if (it->second.address == addr)
    return &it->second;

  if (it != m_functions.begin())
  {
    // Otherwise, check whether the address is within the bounds of a symbol.
    --it;
    if (addr >= it->second.address && addr < it->second.address + it->second.size)
      return &it->second;
  }

@Pokechu22
Copy link
Contributor Author

That has the exact same bug as the old code; m_functions.lower_bound(addr) returns m_functions.end() if the address is past the start of the last function.

@AdmiralCurtiss
Copy link
Contributor

Aaah yup you're right, lower_bound is confusing me. I guess this is fine as-is then.

Copy link
Contributor

@sepalani sepalani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

if (it != m_functions.end())
{
// If the address is exactly the start address of a symbol, we're done.
if (it->second.address == addr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if statement can be combined with the above one: it != m_functions.end() && it->second.address == addr. Feel free to ignore this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a bit more readable this way since the one below needs to be two separate statements, so having this one be two makes it a bit more consistent.

@AdmiralCurtiss AdmiralCurtiss merged commit 92c7566 into dolphin-emu:master Jul 30, 2022
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants