Bug 226401 – Non-unified build fixes, late-ish May 2021 edition redux

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Adrian Perez]

☢️

[Adrian Perez]

Created attachment 430064 [details]
Patch

[EWS]

Committed r278237 (238274@main): <https://commits.webkit.org/238274@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430064 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78646851>

[Mark Lam]

Comment on attachment 430064 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430064&action=review

> Source/JavaScriptCore/jit/JITSizeStatistics.h:31
> +#include "CCallHelpers.h"

This is the wrong thing to do because this header file doesn't really depend on CCallHelpers.h.

The right thing to do is to #include "CCallHelpers.h" in the files that actually uses JITSizeStatistics::markStart and markEnd.  AFAICT, the only files that need it are JIT.cpp and DFGSpeculativeJIT.cpp, and those files already #include CCallHelpers.h indirectly.  So, what is motivating this change?

[Adrian Perez]

(In reply to Mark Lam from comment #4)
> Comment on attachment 430064 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430064&action=review
> 
> > Source/JavaScriptCore/jit/JITSizeStatistics.h:31
> > +#include "CCallHelpers.h"
> 
> This is the wrong thing to do because this header file doesn't really depend
> on CCallHelpers.h.
>
> The right thing to do is to #include "CCallHelpers.h" in the files that
> actually uses JITSizeStatistics::markStart and markEnd.  AFAICT, the only
> files that need it are JIT.cpp and DFGSpeculativeJIT.cpp, and those files
> already #include CCallHelpers.h indirectly.  So, what is motivating this
> change?

In JITSizeStatistics.h there is an use of the CCallHelpers::Label
type, which is defined all the way up the class inheritance chain
in the AbstractMacroAssembler<T>. I gave it a small spin at trying
to figure out the correct “T” to use but it ended up needing to know
the particular type for the current target architecture and anyway
needed including headers that CCallHelpers.h itself is already
including… in the end it seemed easier and much more readable to
keep the CCallHelpers::Label usage as-is and include CCallHelper.h

If anybody has a better suggestion on how to get a definition for
the CCallHelpers::Label type I can try to give it a try.

[Mark Lam]

(In reply to Adrian Perez from comment #5)
> (In reply to Mark Lam from comment #4)
> > Comment on attachment 430064 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=430064&action=review
> > 
> > > Source/JavaScriptCore/jit/JITSizeStatistics.h:31
> > > +#include "CCallHelpers.h"
> > 
> > This is the wrong thing to do because this header file doesn't really depend
> > on CCallHelpers.h.
> >
> > The right thing to do is to #include "CCallHelpers.h" in the files that
> > actually uses JITSizeStatistics::markStart and markEnd.  AFAICT, the only
> > files that need it are JIT.cpp and DFGSpeculativeJIT.cpp, and those files
> > already #include CCallHelpers.h indirectly.  So, what is motivating this
> > change?
> 
> In JITSizeStatistics.h there is an use of the CCallHelpers::Label
> type, which is defined all the way up the class inheritance chain
> in the AbstractMacroAssembler<T>. I gave it a small spin at trying
> to figure out the correct “T” to use but it ended up needing to know
> the particular type for the current target architecture and anyway
> needed including headers that CCallHelpers.h itself is already
> including… in the end it seemed easier and much more readable to
> keep the CCallHelpers::Label usage as-is and include CCallHelper.h
> 
> If anybody has a better suggestion on how to get a definition for
> the CCallHelpers::Label type I can try to give it a try.

I stand corrected.  That's unfortunate.  Thanks for explaining.  We could have just used MacroAssembler::Label, but then, we'd still be #include'ing MacroAssembler.h.  Given this, it's not worth thinking more about this issue.  Your fix is correct.  Sorry for the noise.

[Adrian Perez]

(In reply to Mark Lam from comment #6)
> (In reply to Adrian Perez from comment #5)
> > (In reply to Mark Lam from comment #4)
> > > Comment on attachment 430064 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=430064&action=review
> > > 
> > > > Source/JavaScriptCore/jit/JITSizeStatistics.h:31
> > > > +#include "CCallHelpers.h"
> > > 
> > > This is the wrong thing to do because this header file doesn't really depend
> > > on CCallHelpers.h.
> > >
> > > The right thing to do is to #include "CCallHelpers.h" in the files that
> > > actually uses JITSizeStatistics::markStart and markEnd.  AFAICT, the only
> > > files that need it are JIT.cpp and DFGSpeculativeJIT.cpp, and those files
> > > already #include CCallHelpers.h indirectly.  So, what is motivating this
> > > change?
> > 
> > In JITSizeStatistics.h there is an use of the CCallHelpers::Label
> > type, which is defined all the way up the class inheritance chain
> > in the AbstractMacroAssembler<T>. I gave it a small spin at trying
> > to figure out the correct “T” to use but it ended up needing to know
> > the particular type for the current target architecture and anyway
> > needed including headers that CCallHelpers.h itself is already
> > including… in the end it seemed easier and much more readable to
> > keep the CCallHelpers::Label usage as-is and include CCallHelper.h
> > 
> > If anybody has a better suggestion on how to get a definition for
> > the CCallHelpers::Label type I can try to give it a try.
> 
> I stand corrected.  That's unfortunate.  Thanks for explaining.  We could
> have just used MacroAssembler::Label, but then, we'd still be #include'ing
> MacroAssembler.h.  Given this, it's not worth thinking more about this
> issue.  Your fix is correct.  Sorry for the noise.

No problem at all, that's what we are here for—making WebKit as good
as we can :)