Page MenuHomeWildfire Games

Add basic Clang 6 support
Needs ReviewPublic

Authored by gentz on Oct 9 2017, 2:09 PM.

Details

Reviewers
Itms
Stan
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

Adds basic clang 6 support. Saddly I couldn't get spidermonkey 38 to compile
with clang 6, so instead I compile spidermonkey with gcc.
The latest spidermonkey from the gecko-dev respitory does compile however,
but updating the game to use it will take an painfull amount of time.

Diff 3856 shows my attempts at upgrading to the latest, however I got stuck on
replacing the [gs]etProperty functions from the JSClasses to proxies because
A) I have no clue what 0ad's code does B) I have no clue what the
proxies do and C) After spending a couple weeks I'm sick of updating code.

Test Plan

Well... it compiles.

One issue happens when starting a singleplayer (haven't tested multi) game:
Lots of red errors scroll down the top left of the screen when starting a
game, however it also happens in the GCC build, so I assume its a regression
from another change. Errors have something to do with loading house sprites
or something.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 3365
Build 5829: Vulcan BuildJenkins
Build 5828: arc lint + arc unit

Event Timeline

gentz created this revision.Oct 9 2017, 2:09 PM
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Oct 9 2017, 2:09 PM

Saddly I couldn't get spidermonkey 38 to compile with clang 6, so instead I compile spidermonkey with gcc.

Have you tried with the SM45 wip branch (https://github.com/leper/0ad/commits/sm45)? That might need a lot of updating, and last I checked it just barely compiled with gcc (in release mode that is), so I'm not sure if clang 6 would be any better there. (That branch also doesn't run very well since there is either a bug in our GUI JS code, or in that SpiderMonkey version.)
But it would be a lot better to try to make that thing work, instead of focussing on SM38.

The latest spidermonkey from the gecko-dev respitory does compile however, but updating the game to use it will take an painfull amount of time.

Yes, SpiderMonkey upgrades take lots of time and patching, and aren't fun. And we are still limited to actually released versions, of which 45 is still the latest (unless upstream contrary to previous experience actually released 52 and just forgot to update the documentation).

build/workspaces/build-clang6.sh
7 ↗(On Diff #3864)

Apart from this specific line being pointless, the whole file seems quite pointless, since you can just export CC and CXX before calling the below.

libraries/source/fcollada/src/FCollada/FCDocument/FCDEffectPassState.cpp
44 ↗(On Diff #3864)

That sounds like it fixes some warning about renderState always being greater or equal than 0 since it is presumably some unsigned type.

Or is there some kind of error with clang 6? In any case a build log would be nice.

libraries/source/fcollada/src/FCollada/FCDocument/FCDExtra.h
444 ↗(On Diff #3864)

Useless change.

libraries/source/fcollada/src/FCollada/FMath/FMArray.h
300 ↗(On Diff #3864)

Is this actually an error or just a warning? (Not that we shouldn't fix it.)

libraries/source/fcollada/src/Makefile
11

Is there a need to specify a specific version? And if we have to specify one, go with c++11 since we do have to support old compilers (see https://trac.wildfiregames.com/wiki/CppSupport).

libraries/source/spidermonkey/build.sh
20

It does, just not with as of yet unreleased versions. Though I must admit that I haven't tried with clang 5 yet.

21

This seems bad, given that this is very likely to not be removed once we fixed the underlying issue.

Also just doing CC=gcc CXX=g++ ./update-workspaces.sh, or just doing CC=clang CXX=clang++ make seems easier than this.

gentz updated this revision to Diff 3872.Oct 9 2017, 11:02 PM

Fixes comments

gentz added inline comments.Oct 9 2017, 11:02 PM
build/workspaces/build-clang6.sh
7 ↗(On Diff #3864)

Ok, I've removed it.

libraries/source/fcollada/src/FCollada/FCDocument/FCDEffectPassState.cpp
44 ↗(On Diff #3864)

Yes, this gives the warning:

FCollada/FCDocument/FCDEffectPassState.cpp:44:18: warning: comparison of unsigned enum expression >= 0 is always true [-Wtautological-unsigned-enum-zero-compare]
        if (renderState >= 0 && renderState < FUDaePassState::COUNT) dataSize = dataSizeTable[type];
            ~~~~~~~~~~~ ^  ~

I've removed this change as it's not strictly necessary for getting it to compile with clang 6, however it would be nice if these warning and others were fixed in another bug.

libraries/source/fcollada/src/FCollada/FCDocument/FCDExtra.h
444 ↗(On Diff #3864)

Removed, didn't notice I did that.

libraries/source/fcollada/src/FCollada/FMath/FMArray.h
300 ↗(On Diff #3864)

This gives the warning:

FCollada/FMath/FMArray.h:300:14: warning: destination for this 'memcpy' call is a pointer to dynamic class 'FUTrackedList<FCDAnimationCurve>'; vtable pointer will be overwritten [-Wdynamic-class-memaccess]
                                                memcpy(newValues, heapBuffer, sized * sizeof(T));
                                                ~~~~~~ ^
FCollada/FMath/FMArray.h:229:4: note: in instantiation of member function 'fm::vector<FUTrackedList<FCDAnimationCurve>, false>::reserve' requested here
                        reserve(count);
                        ^
FCollada/FCDocument/FCDAnimated.cpp:38:9: note: in instantiation of member function 'fm::vector<FUTrackedList<FCDAnimationCurve>, false>::resize' requested here
        curves.resize(valueCount);
               ^
FCollada/FMath/FMArray.h:300:14: note: explicitly cast the pointer to silence this warning
                                                memcpy(newValues, heapBuffer, sized * sizeof(T));
                                                       ^
                                                       (void*)

I've removed this change as it's not strictly necessary for getting it to compile with clang 6, however it would be nice if these warning and others were fixed in another bug.

libraries/source/fcollada/src/Makefile
11

It's not necessary to use C++17. I've changed it to 11.

libraries/source/spidermonkey/build.sh
20

I've not tested clang 5 either. I've updated the code to do this only when using clang 6.

21

My goal is to compile as much as I can with clang 6, not passing the CC and CXX variables to ./update-workspaces.sh wouldn't compile NVTT and FCollada with clang 6.

I could make it echo a warning, but I don't think that will help.

Vulcan added a subscriber: Vulcan.Oct 9 2017, 11:20 PM

Build is green

Updating workspaces...
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimation]’:
FCollada/FCDocument/FCDLibrary.cpp:149:30:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
  const T* cptr = ((const FCDLibrary<T>*)l1)->GetEntity(0);
           ^
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimationClip]’:
FCollada/FCDocument/FCDLibrary.cpp:150:34:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDCamera]’:
FCollada/FCDocument/FCDLibrary.cpp:151:27:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDController]’:
FCollada/FCDocument/FCDLibrary.cpp:152:31:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEffect]’:
FCollada/FCDocument/FCDLibrary.cpp:153:27:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEmitter]’:
FCollada/FCDocument/FCDLibrary.cpp:154:28:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDForceField]’:
FCollada/FCDocument/FCDLibrary.cpp:155:31:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDGeometry]’:
FCollada/FCDocument/FCDLibrary.cpp:156:29:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDImage]’:
FCollada/FCDocument/FCDLibrary.cpp:157:26:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDLight]’:
FCollada/FCDocument/FCDLibrary.cpp:158:26:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDMaterial]’:
FCollada/FCDocument/FCDLibrary.cpp:159:29:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDSceneNode]’:
FCollada/FCDocument/FCDLibrary.cpp:160:30:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsModel]’:
FCollada/FCDocument/FCDLibrary.cpp:161:33:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsMaterial]’:
FCollada/FCDocument/FCDLibrary.cpp:162:36:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsScene]’:
FCollada/FCDocument/FCDLibrary.cpp:163:33:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
In file included from FCollada/FUtils/FUSemaphore.cpp:10:0:
FCollada/FUtils/FUSemaphore.h:36:2: warning: #warning "FUSemaphore: Semaphore not implemented for non Windows" [-Wcpp]
 #warning "FUSemaphore: Semaphore not implemented for non Windows"
  ^
FCollada/FUtils/FUStringConversion.cpp: In function ‘void TrickLinkerFUStringConversion()’:
FCollada/FUtils/FUStringConversion.cpp:281:8: warning: variable ‘f’ set but not used [-Wunused-but-set-variable]
  float f = FUStringConversion::ToFloat(&c);
        ^
FCollada/FUtils/FUStringConversion.cpp:283:7: warning: variable ‘b’ set but not used [-Wunused-but-set-variable]
  bool b = FUStringConversion::ToBoolean(c);
       ^
FCollada/FUtils/FUStringConversion.cpp:285:8: warning: variable ‘i32’ set but not used [-Wunused-but-set-variable]
  int32 i32 = FUStringConversion::ToInt32(&c);
        ^
FCollada/FUtils/FUStringConversion.cpp:287:9: warning: variable ‘u32’ set but not used [-Wunused-but-set-variable]
  uint32 u32 = FUStringConversion::ToUInt32(&c);
         ^
In file included from FCollada/FUtils/FUThread.cpp:10:0:
FCollada/FUtils/FUThread.h:30:2: warning: #warning "Threads not yet implemented for non Windows." [-Wcpp]
 #warning "Threads not yet implemented for non Windows."
  ^
FCollada/FCDocument/FCDAnimationCurve.cpp: In member function ‘float FCDAnimationCurve::Evaluate(float) const’:
FCollada/FCDocument/FCDAnimationCurve.cpp:396:13: warning: ‘inTangent.FMVector2::<anonymous>.FMVector2::<anonymous union>::x’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   FMVector2 inTangent;
             ^
FCollada/FCDocument/FCDAnimationCurve.cpp:396:13: warning: ‘inTangent.FMVector2::<anonymous>.FMVector2::<anonymous union>::y’ may be used uninitialized in this function [-Wmaybe-uninitialized]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimation]’:
FCollada/FCDocument/FCDLibrary.cpp:149:30:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
  const T* cptr = ((const FCDLibrary<T>*)l1)->GetEntity(0);
           ^
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimationClip]’:
FCollada/FCDocument/FCDLibrary.cpp:150:34:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDCamera]’:
FCollada/FCDocument/FCDLibrary.cpp:151:27:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDController]’:
FCollada/FCDocument/FCDLibrary.cpp:152:31:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEffect]’:
FCollada/FCDocument/FCDLibrary.cpp:153:27:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEmitter]’:
FCollada/FCDocument/FCDLibrary.cpp:154:28:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDForceField]’:
FCollada/FCDocument/FC

http://jenkins-master:8080/job/phabricator/2113/ for more details.

gentz added a comment.Oct 10 2017, 1:03 AM

Sorry for taking to long to get back on this.

In D953#37241, @leper wrote:

Saddly I couldn't get spidermonkey 38 to compile with clang 6, so instead I compile spidermonkey with gcc.

Have you tried with the SM45 wip branch (https://github.com/leper/0ad/commits/sm45)? That might need a lot of updating, and last I checked it just barely compiled with gcc (in release mode that is), so I'm not sure if clang 6 would be any better there. (That branch also doesn't run very well since there is either a bug in our GUI JS code, or in that SpiderMonkey version.)
But it would be a lot better to try to make that thing work, instead of focussing on SM38.

Spidermokey 45, ike spidermonkey 38, also fails to link.
The build log for SM45: https://code.wildfiregames.com/P88

One note: SM35 only has linking issues with some (all?) functions in libm.so (or maybe ones which are defined in cmath/math.h), examples being functions like pow, floor and acos, while SM45 has linker issues with those math functions and memory management functions (malloc, free, ect).

For people in the future who want to attempt to fix the linker issues, here is what I've tried so far.

  • Using ld.lld instead of ld.gold
  • Passing -fpic and -fPIC as both a CXX flags and a LD flag
  • Passing -Wl,-no-as-needed as a LD flag
  • Passing -Wl,-no-undefined as a LD flag
  • Adding the std:: namespace in front of functions with linking issues.
  • Including cmath
  • Including math.h
  • Passing -std=c++17 as a CXX flag

The latest spidermonkey from the gecko-dev repository does compile however, but updating the game to use it will take an painful amount of time.

Yes, SpiderMonkey upgrades take lots of time and patching, and aren't fun. And we are still limited to actually released versions, of which 45 is still the latest (unless upstream contrary to previous experience actually released 52 and just forgot to update the documentation).

I also think 45 is the latest, but that's just a guess.

gentz updated this revision to Diff 3874.Oct 10 2017, 1:11 AM

Fixed small typo

Spidermokey 45, ike spidermonkey 38, also fails to link.
The build log for SM45: https://code.wildfiregames.com/P88

fcollada is quite noisy, can't blame you for fixing a few warnings, though I guess we should look at that memcpy usage a bit more (and only add one cast if it turns out to be ok).

One note: SM35 only has linking issues with some (all?) functions in libm.so (or maybe ones which are defined in cmath/math.h), examples being functions like pow, floor and acos, while SM45 has linker issues with those math functions and memory management functions (malloc, free, ect).

That does seem eerily familiar, though my memory seems to fail me if I encountered that with SM.

But as it is a linker issue, I'd suggest injecting -lm into the linker command parameters. If that does not fix it, try looking for some object files (nm might be helpful) somewhere in the SM build folders that provide those symbols.

(I guess I'd go with somewhat hackily patching that in manually after the SM source was extracted, and if that fixes it worry about making that work nicely later.)

leper added inline comments.Oct 10 2017, 1:39 AM
libraries/source/spidermonkey/build.sh
21

The slightly ugly way to do that would be to first build with clang 6, get fcollada and nvtt to build, SM to fail, then build with gcc.

Not elegant, but IMO this hunk serves no purpose, at least once we actually get SM to build. (Be that after lots of patching, or upgrading and lots of patching. (This is SM, there is no way that doesn't involve patching.))

Build is green

Updating workspaces...
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimation]’:
FCollada/FCDocument/FCDLibrary.cpp:149:30:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
  const T* cptr = ((const FCDLibrary<T>*)l1)->GetEntity(0);
           ^
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimationClip]’:
FCollada/FCDocument/FCDLibrary.cpp:150:34:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDCamera]’:
FCollada/FCDocument/FCDLibrary.cpp:151:27:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDController]’:
FCollada/FCDocument/FCDLibrary.cpp:152:31:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEffect]’:
FCollada/FCDocument/FCDLibrary.cpp:153:27:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEmitter]’:
FCollada/FCDocument/FCDLibrary.cpp:154:28:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDForceField]’:
FCollada/FCDocument/FCDLibrary.cpp:155:31:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDGeometry]’:
FCollada/FCDocument/FCDLibrary.cpp:156:29:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDImage]’:
FCollada/FCDocument/FCDLibrary.cpp:157:26:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDLight]’:
FCollada/FCDocument/FCDLibrary.cpp:158:26:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDMaterial]’:
FCollada/FCDocument/FCDLibrary.cpp:159:29:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDSceneNode]’:
FCollada/FCDocument/FCDLibrary.cpp:160:30:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsModel]’:
FCollada/FCDocument/FCDLibrary.cpp:161:33:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsMaterial]’:
FCollada/FCDocument/FCDLibrary.cpp:162:36:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ s

http://jenkins-master:8080/job/phabricator/2115/ for more details.

gentz added a comment.Oct 10 2017, 4:23 AM
In D953#37274, @leper wrote:

(and only add one cast if it turns out to be ok).

You'd need both casts, as after you cast the first one, it gives a similar warning for the second.

One note: SM35 only has linking issues with some (all?) functions in libm.so (or maybe ones which are defined in cmath/math.h), examples being functions like pow, floor and acos, while SM45 has linker issues with those math functions and memory management functions (malloc, free, ect).

That does seem eerily familiar, though my memory seems to fail me if I encountered that with SM.

But as it is a linker issue, I'd suggest injecting -lm into the linker command parameters.

Its already in the arguments, twice. I've already tried adding it a third time to no avail. :/

If that does not fix it, try looking for some object files (nm might be helpful) somewhere in the SM build folders that provide those symbols.

Nothing provides them:

  ~/Do…ts/0adclang6                                                                                                 0|0|INT(-2) ↵  1.77281   0.79    6.79G   8%  
  20:16  09.10.17  find libraries/source/spidermonkey | grep "\.o$" | xargs nm | grep pow
                 U pow
                 U _ZN2js15math_pow_handleEP9JSContextN2JS6HandleINS2_5ValueEEES5_NS2_13MutableHandleIS4_EE
                 U _ZN2js8math_powEP9JSContextjPN2JS5ValueE
                 U pow
0000000000000000 W _ZN2js3jit5LPowD5powerEv
0000000000000000 W _ZN2js3jit5LPowI5powerEv
                 U _ZN2js4powiEdi
0000000000000000 W _ZSt3powIdiEN9__gnu_cxx11__promote_2IT_T0_NS0_9__promoteIS2_Xsr3std12__is_integerIS2_EE7__valueEE6__typeENS4_IS3_Xsr3std12__is_integerIS3_EE7__valueEE6__typeEE6__typeES2_S3_
                 U _ZN2js8math_powEP9JSContextjPN2JS5ValueE
0000000000000000 W _ZNK2js3jit4MPow5powerEv
00000000000000b0 r _ZL10powersOf10
000000000004d2f0 t _ZL8pow5multP9DtoaStateP6Biginti
0000000000000f18 r _ZZL8pow5multP9DtoaStateP6BigintiE3p05
                 U pow
0000000000001e60 T _ZN2js15math_pow_handleEP9JSContextN2JS6HandleINS2_5ValueEEES5_NS2_13MutableHandleIS4_EE
0000000000001aa0 T _ZN2js4powiEdi
0000000000001f10 T _ZN2js8math_powEP9JSContextjPN2JS5ValueE
0000000000000770 r _ZN17double_conversionL19exact_powers_of_tenE

  ~/Do…ts/0adclang6                                                                                                               7.67316   0.77    6.79G   8%  
  20:16  09.10.17  find libraries/source/spidermonkey | grep "\.o$" | xargs nm | grep acos
                 U _ZN2js10math_acoshEP9JSContextjPN2JS5ValueE
                 U _ZN2js18math_acos_uncachedEd
                 U _ZN2js19math_acosh_uncachedEd
                 U _ZN2js9math_acosEP9JSContextjPN2JS5ValueE
                 U _ZN2js14math_acos_implEPNS_9MathCacheEd
                 U _ZN2js15math_acosh_implEPNS_9MathCacheEd
                 U _ZN2js18math_acos_uncachedEd
                 U _ZN2js19math_acosh_uncachedEd
                 U acos
                 U _ZN2js9math_acosEP9JSContextjPN2JS5ValueE
                 U acos
                 U acosh
0000000000003a90 t _ZL13math_functionIXadL_ZN2js15math_acosh_implEPNS0_9MathCacheEdEEEbP9JSContextjPN2JS5ValueE
0000000000003a60 T _ZN2js10math_acoshEP9JSContextjPN2JS5ValueE
0000000000000360 T _ZN2js14math_acos_implEPNS_9MathCacheEd
0000000000003a00 T _ZN2js15math_acosh_implEPNS_9MathCacheEd
00000000000003a0 T _ZN2js18math_acos_uncachedEd
0000000000003a40 T _ZN2js19math_acosh_uncachedEd
00000000000003c0 T _ZN2js9math_acosEP9JSContextjPN2JS5ValueE

  ~/Do…ts/0adclang6                                                                                                               7.77101   1.30    6.78G   8%  
  20:17  09.10.17  find . | grep "\.o$" | xargs nm | grep acos                            
                 U acosf
                 U acosf
0000000000000000 W _ZSt4acosf
                 U _ZN2js10math_acoshEP9JSContextjPN2JS5ValueE
                 U _ZN2js18math_acos_uncachedEd
                 U _ZN2js19math_acosh_uncachedEd
                 U _ZN2js9math_acosEP9JSContextjPN2JS5ValueE
                 U _ZN2js14math_acos_implEPNS_9MathCacheEd
                 U _ZN2js15math_acosh_implEPNS_9MathCacheEd
                 U _ZN2js18math_acos_uncachedEd
                 U _ZN2js19math_acosh_uncachedEd
                 U acos
                 U _ZN2js9math_acosEP9JSContextjPN2JS5ValueE
                 U acos
                 U acosh
0000000000003a90 t _ZL13math_functionIXadL_ZN2js15math_acosh_implEPNS0_9MathCacheEdEEEbP9JSContextjPN2JS5ValueE
0000000000003a60 T _ZN2js10math_acoshEP9JSContextjPN2JS5ValueE
0000000000000360 T _ZN2js14math_acos_implEPNS_9MathCacheEd
0000000000003a00 T _ZN2js15math_acosh_implEPNS_9MathCacheEd
00000000000003a0 T _ZN2js18math_acos_uncachedEd
0000000000003a40 T _ZN2js19math_acosh_uncachedEd
00000000000003c0 T _ZN2js9math_acosEP9JSContextjPN2JS5ValueE

libm:

 20:20  09.10.17  readelf -Ws /usr/lib/libm-2.26.so | grep pow
   42: 0000000000048590  1695 FUNC    GLOBAL DEFAULT   11 __powf_finite@@GLIBC_2.15
  143: 000000000005e710  4275 FUNC    GLOBAL DEFAULT   11 __powf128_finite@@GLIBC_2.26
  198: 000000000000dc10   298 FUNC    WEAK   DEFAULT   11 powf@@GLIBC_2.2.5
  202: 0000000000009050   510 FUNC    WEAK   DEFAULT   11 powl@@GLIBC_2.2.5
  315: 0000000000052720   158 FUNC    WEAK   DEFAULT   11 cpowf@@GLIBC_2.2.5
  321: 000000000001b9a0   101 FUNC    WEAK   DEFAULT   11 cpowl@@GLIBC_2.2.5
  322: 0000000000028f10    40 IFUNC   GLOBAL DEFAULT   11 __pow_finite@@GLIBC_2.15
  345: 000000000000d6e0   126 FUNC    WEAK   DEFAULT   11 pow10f@@GLIBC_2.2.5
  350: 00000000000088a0   138 FUNC    WEAK   DEFAULT   11 pow10l@@GLIBC_2.2.5
  367: 000000000000a020   122 FUNC    WEAK   DEFAULT   11 pow10@@GLIBC_2.2.5
  394: 0000000000070f80   172 FUNC    WEAK   DEFAULT   11 cpowf128@@GLIBC_2.26
  485: 00000000000441b0    48 FUNC    WEAK   DEFAULT   11 cpow@@GLIBC_2.2.5
  501: 000000000000a580   336 FUNC    WEAK   DEFAULT   11 pow@@GLIBC_2.2.5
  593: 0000000000072a70   522 FUNC    WEAK   DEFAULT   11 powf128@@GLIBC_2.26
  620: 00000000000116b0   843 FUNC    GLOBAL DEFAULT   11 __powl_finite@@GLIBC_2.15

Its already in the arguments, twice. I've already tried adding it a third time to no avail. :/

Can reproduce with SM38 and clang 5 locally, I guess I'll give this a go.

Apart from that I guess I'll commit parts of this (the fcollada fix) sometime soon. If you want you can submit fixes for those warnings in a different patch.

Do you still have that git(?) using thing around? If yes, care to test if the fix occured in April 2016? (At least that is when some of the related code changed https://bugzilla.mozilla.org/show_bug.cgi?id=933257.)

gentz added a comment.Oct 11 2017, 7:48 AM
In D953#37300, @leper wrote:

Do you still have that git(?) using thing around? If yes, care to test if the fix occured in April 2016? (At least that is when some of the related code changed https://bugzilla.mozilla.org/show_bug.cgi?id=933257.)

Both change set "b70ae970d45d" (Part 1's parent) and change set "336759fb7df0" (Part 9) fail with the linker errors mentioned previously. So it was fixed earlier than that,.
My guess is sometime in the last 18 months they added a random include statement for a random header in some other random header, which header, where they added it and when is beyond me.

gentz added a comment.Oct 11 2017, 8:08 AM

I've started git bisecting the issue, gimme a couple days at most to finish bisecting.

Both change set "b70ae970d45d" (Part 1's parent) and change set "336759fb7df0" (Part 9) fail with the linker errors mentioned previously. So it was fixed earlier than that,.

later, not earlier.

My guess is sometime in the last 18 months they added a random include statement for a random header in some other random header, which header, where they added it and when is beyond me.

Something somewhere changed, that is for certain. If it is just a single commit (or at least a few grouped together) that should be easy enough to backport.

Thanks for doing the heavy lifting of bisecting spidermonkey!

In D953#37332, @leper wrote:

Both change set "b70ae970d45d" (Part 1's parent) and change set "336759fb7df0" (Part 9) fail with the linker errors mentioned previously. So it was fixed earlier than that,.

later, not earlier.

My bad.

Thanks for doing the heavy lifting of bisecting spidermonkey!

And I've found it. The commit which fixes the issue is 93ef406a765595d3a28422a2861964945a567702
"Bug 1279369 - Move --enable-debug, MOZ_DEBUG_FLAGS, and --enable-debug-symbols to Python configure. r=glandium"
https://bugzilla.mozilla.org/show_bug.cgi?id=1279369

I'll leave it up to you to figure out what changes are necessary.

Thanks for doing the heavy lifting of bisecting spidermonkey!

And I've found it. The commit which fixes the issue is 93ef406a765595d3a28422a2861964945a567702
"Bug 1279369 - Move --enable-debug, MOZ_DEBUG_FLAGS, and --enable-debug-symbols to Python configure. r=glandium"
https://bugzilla.mozilla.org/show_bug.cgi?id=1279369

So that fixes it by switching to the new build system... I guess backporting that is possible, but actually upgrading to something newer might be less work in the long run.

I'll leave it up to you to figure out what changes are necessary.

I guess I'll commit the non-SpiderMonkey fixes sometime and consider SpiderMonkey broken on recent clang and do nothing about it. (Maybe @Itms is interested in making that work.)

wraitii added a reviewer: Restricted Owners Package.May 14 2018, 4:40 PM
Stan requested changes to this revision.Dec 16 2019, 9:35 AM
Stan added a subscriber: Stan.

SpiderMonkey 45 is in. So this shouldn't be needed?

This revision now requires changes to proceed.Dec 16 2019, 9:35 AM
Stan resigned from this revision.Dec 16 2019, 9:35 AM
This revision now requires review to proceed.Dec 16 2019, 9:35 AM