Open Bug 1490743 Opened 1 year ago Updated 3 months ago

[clang, visual studio debugging] Identification of a concrete class in a general pointer variable doesn't work any more

Categories

(Firefox Build System :: Toolchains, defect, blocker)

x86_64
Windows 10
defect
Not set
blocker

Tracking

(Not tracked)

People

(Reporter: mayhemer, Unassigned)

References

Details

Attachments

(2 obsolete files)

STR:
- Windows 10, up to date m-c, bootstrapped
- build m-c with .mozconfig [1]
- put a breakpoint in visual studio at [2], expected to hit on the parent process only
- run firefox.exe from inside the vs ide (or attach, will likely make no difference)
- the breakpoint will likely hit very soon, or just load an arbitrary page, if not
- hover over or locate in the Autos window the mURI (nsCOMPtr<nsIURI>) member of the class (declared in HttpChannelBase base class)
- try to examine the mRawPtr (the final URI class)

ER:
- nsHttpChannel->mURI.mRawPtr is revealed as nsStandardURL class with its members

AR:
- nsHttpChannel->mURI.mRawPtr is only seen as nsISupports (not even as nsIURI!) and I can't examine the content

Note that adding (nsStandardURL*)(value of mRawPtr) to Watches shows what's expected, confirming the address is correct.


Filing as a blocker as I really can't work now.  Feel free to drop the severity, but I would appreciate some look at this soon.


[1] 
ac_add_options --build-backends=-VisualStudio
ac_add_options --host=x86_64-pc-mingw32
ac_add_options --target=x86_64-pc-mingw32
export MOZ_DEBUG_SYMBOLS=1
ac_add_options --enable-warnings-as-errors
ac_add_options --enable-application=browser
ac_add_options --enable-debug
ac_add_options --disable-optimize
ac_add_options --disable-crashreporter
ac_add_options --enable-tests
ac_add_options --enable-chrome-format=flat


[2] https://searchfox.org/mozilla-central/rev/9e7995b3c384945740985107a4de601b27904718/netwerk/protocol/http/nsHttpChannel.cpp#3912
my VS IDE version is 15.8.4 (the latest)
In general it seems to me that the debugger now simply uses the first base class of the type of the variable.  I've also seen this: an argument of type nsAHttpTransaction* keeping nsHttpTransaction final class translates to nsSupportsWeakReference* - the first base class of nsAHttpTransaction.
Blocks: 1483835
(In reply to Honza Bambas (:mayhemer) from comment #2)
> In general it seems to me that the debugger now simply uses the first base
> class of the type of the variable.  I've also seen this: an argument of type
> nsAHttpTransaction* keeping nsHttpTransaction final class translates to
> nsSupportsWeakReference* - the first base class of nsAHttpTransaction.

Confirmed in WinDbg too: we're not being offered `vtcast` views of these pointers. MSVC on the left, clang on the right: https://irccloud.mozilla.com/file/EgMPnKIR/vtcast (This was from the Win64 debug builds from https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&selectedJob=198925654&revision=0b59dc25e8749f34590f2e5b29f3906f98f29730)
I filed this upstream as https://bugs.llvm.org/show_bug.cgi?id=38944.
Attached patch Possible workaround (obsolete) — Splinter Review
Per LLVM's IRC, -fno-limit-debug-info might work around the issue. Honza, does this improve things for you?
Attachment #9008915 - Flags: feedback?(honzab.moz)
Comment on attachment 9008915 [details] [diff] [review]
Possible workaround

Yes, this fixes the problem!  Thanks.  It seems (unconfirmed in more scientific way) to slightly prolong build times.
Attachment #9008915 - Flags: feedback?(honzab.moz) → feedback+
Comment on attachment 9008915 [details] [diff] [review]
Possible workaround

This workaround seems to stop working recently again.

I can give STR if wanted.
Attachment #9008915 - Flags: feedback+ → feedback-
Flags: needinfo?(dmajor)
(In reply to Honza Bambas (:mayhemer) from comment #7)
> Comment on attachment 9008915 [details] [diff] [review]
> Possible workaround
> 
> This workaround seems to stop working recently again.
> 
> I can give STR if wanted.

Yes please.
Flags: needinfo?(dmajor)
(In reply to David Major [:dmajor] from comment #8)
> (In reply to Honza Bambas (:mayhemer) from comment #7)
> > Comment on attachment 9008915 [details] [diff] [review]
> > Possible workaround
> > 
> > This workaround seems to stop working recently again.
> > 
> > I can give STR if wanted.
> 
> Yes please.

Please consider comment 7 unconfirmed now.  I can't reproduce any more during my debug sessions with the patch applied and lost the STR from comment 7 already.

Is there anything that would keep this from finally landing?

David, can we please land this? I used to have this patch in mq, but now I use hg bookmarks - it's hard to have custom patches with that workflow.

This is a valid workaround for a clang bug and NOT having it in the tree actually disallows me from doing much work. Thanks.

Flags: needinfo?(dmajor)

I'm not comfortable landing this change during code freeze. Upstream says this flag comes with a substantial cost in file size and build speed. Remember that this flag is a big-hammer workaround for a bug, and not a true "fix".

Ideally the upstream bug would get properly fixed -- I'll prod it. Failing that, maybe we can try this early next cycle, but in the meantime you should be able to export CFLAGS in your mozconfig regardless of mq/bookmarks/etc.

Flags: needinfo?(dmajor)

Thanks for the tip. Adding:

mk_add_options "export CFLAGS=-fno-limit-debug-info"
mk_add_options "export CXXFLAGS=-fno-limit-debug-info"

to my mozconfig solves the problem locally for me.

Please advise how to resolve this bug (WONTFIX, INCOMPLETE or somehow leave open dependent on the clang issue)

Now that we have a fresh cycle, I'm willing to give it a try. We'll see if the sheriffs yell at me...

See https://bugs.llvm.org/show_bug.cgi?id=38944. clang thought it could safely leave out type information for some of our classes, but that information would have been helpful for debugging. The flag in this patch turns off the debug-info limiting.

Comment on attachment 9008915 [details] [diff] [review]
Possible workaround

Confirmed that adding

mk_add_options "export CFLAGS=-fno-limit-debug-info"
mk_add_options "export CXXFLAGS=-fno-limit-debug-info" 

to my mozconfig fixes the problem.
Attachment #9008915 - Flags: feedback- → feedback+
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/70ce7207292e
Work around missing type information in Windows debug symbols r=chmanchester
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Depends on: 1538189
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Backed out changeset 70ce7207292e (bug 1490743) for increasing build times on Windows platforms. a=backout

Backout link: https://hg.mozilla.org/mozilla-central/rev/df256c98c3fdb56de9bc2900803a6c4d58671100

https://treeherder.mozilla.org/perf.html#/alerts?id=20008

Flags: needinfo?(dmajor)
Attachment #9052073 - Attachment is obsolete: true
Flags: needinfo?(dmajor)
Attachment #9008915 - Attachment is obsolete: true
Assignee: dmajor → nobody
Status: REOPENED → NEW
Target Milestone: mozilla68 → ---
Version: 64 Branch → unspecified
No longer depends on: 1538189
Regressions: 1538189
You need to log in before you can comment on or make changes to this bug.