New issue
Advanced search Search tips
Starred by 2 users

Issue metadata

Status: Duplicate
Merged: issue 756175
Owner: ----
Closed: Sep 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Bug

Blocking:
issue 636111



Sign in to add a comment

Clang Windows symbols jump around for multiline conditionals

Project Member Reported by brettw@chromium.org, Aug 25 2017

Issue description

Stepping and breakpoints for multiline conditionals are surprising with Clang on Windows.

The particular code I noticed this on was:

  if (!thumbnail_service ||
      !thumbnail_service->ShouldAcquirePageThumbnail(url, page_transition_)) {
    return;
  }

from https://cs.chromium.org/chromium/src/chrome/browser/thumbnails/thumbnail_tab_helper.cc?l=197

I set a breakpoint on the if condition and that breakpoint gets hit twice through the code, which was really confusing!

The assembly looks like this:

      // Skip if we don't need to update the thumbnail.
      if (!thumbnail_service ||
 call        scoped_refptr<thumbnails::ThumbnailService>::operator bool
 test        al,1  
 jne         ThumbnailTabHelper::UpdateThumbnailIfNecessary+237h (193EBC97h)  
 jmp         ThumbnailTabHelper::UpdateThumbnailIfNecessary+269h (193EBCC9h) 
          !thumbnail_service->ShouldAcquirePageThumbnail(url, page_transition_)) {
 lea         ecx,[thumbnail_service]  
 ...
 call        eax  

      // Skip if we don't need to update the thumbnail.
      if (!thumbnail_service ||
 test        al,1  
 jne         ThumbnailTabHelper::UpdateThumbnailIfNecessary+275h (193EBCD5h)  
        return;
 mov         dword ptr [esi+3Ch],1  
 jmp         ThumbnailTabHelper::UpdateThumbnailIfNecessary+2A4h (193EBD04h)  
      }

The symbols are blaming the "test al,1" and the "jne" associated with the condition back to the first line of the if statement. This makes theoretical sense but isn't what you expect when stepping through the program.

Testing with VC, it looks like it groups the entire if condition into one "statement" which you "Step" takes you over (Clang shows the two lines of the condition as separate statements). I'm not sure which of these schemes is better, it may depend on what you're trying to do.


      // Skip if we don't need to update the thumbnail.
      if (!thumbnail_service ||
 lea         ecx,[thumbnail_service]  
 call        scoped_refptr<thumbnails::ThumbnailService>::operator bool
 movzx       edx,al  
 test        edx,edx  
 je          ThumbnailTabHelper::UpdateThumbnailIfNecessary+1B3h
 lea         ecx,[thumbnail_service]  
 ...
 test        ecx,ecx  
 jne         ThumbnailTabHelper::UpdateThumbnailIfNecessary+1BDh
          !thumbnail_service->ShouldAcquirePageThumbnail(url, page_transition_)) {
        return;
 lea         ecx,[thumbnail_service]  
 call        scoped_refptr<thumbnails::ThumbnailService>::~scoped_refptr<thumbnails::ThumbnailService>
 jmp         ThumbnailTabHelper::UpdateThumbnailIfNecessary+1D9h
      }

I think the solution is to always blame the corresponding test/jump lines of a condition to the immediately previous statement that they're testing, rather than the if statement as a whole.
 

Comment 1 by brettw@chromium.org, Aug 25 2017

Description: Show this description

Comment 2 by brettw@chromium.org, Aug 25 2017

Summary: Clang Windows symbols jump around for multiline conditionals (was: Clang Windows symbols sump around fo multiline conditionals)

Comment 3 by brettw@chromium.org, Aug 25 2017

Description: Show this description

Comment 4 by brettw@chromium.org, Aug 25 2017

Description: Show this description

Comment 5 Deleted

Comment 6 Deleted

Mergedinto: 756175
Status: Duplicate (was: Untriaged)
This is the same underlying issue as  bug #756175 , which we're already working on a fix for, so I'm duping it into there.

TL;DR is that we emit more line info than cl (it helps in sanitizer traces, among other things), but MSVC debugger doesn't handle it well.  We're going to revert to matching MSVC's behavior for now, but we're also going to open a discussion with MS to see if they can make the debugger experience work better for us (I have some concrete ideas on how to do it, but it's up to them since it requires changing their debugger).

Sign in to add a comment