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

deps/v8/src/objects.h:3263:46: error: invalid use of incomplete type ‘class v8::internal::Heap #10388

Closed
octoploid opened this issue Dec 21, 2016 · 26 comments
Labels
build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency.

Comments

@octoploid
Copy link

octoploid commented Dec 21, 2016

  • Version:
  • Platform:
  • Subsystem:

Compilation fails with gcc-7:

 % g++ '-DV8_TARGET_ARCH_X64' '-DENABLE_DISASSEMBLER' '-DV8_I18N_SUPPORT' -I../deps/v8 -I../. -pthread -Wall -Wextra -Wno-unused-parameter -m64 -fno-strict-aliasing -m64 -fdata-sections -ffunction-sections -O3 -O3 -fno-omit-frame-pointer -fno-rtti -fno-exceptions -std=gnu++0x -MMD -MF /home/markus/tmp/node/out/Release/.deps//home/markus/tmp/node/out/Release/obj.target/v8_nosnapshot/gen/libraries.o.d.raw -c -o /home/markus/tmp/node/out/Release/obj.target/v8_nosnapshot/gen/libraries.o /home/markus/tmp/node/out/Release/obj/gen/libraries.cc

In file included from ../deps/v8/src/snapshot/natives.h:8:0,
                 from /home/markus/tmp/node/out/Release/obj/gen/libraries.cc:8:
../deps/v8/src/objects.h: In member function ‘uint32_t v8::internal::HashTable<Derived, Shape, Key>::Hash(Key)’:
../deps/v8/src/objects.h:3263:46: error: invalid use of incomplete type ‘class v8::internal::Heap’
       return Shape::SeededHash(key, GetHeap()->HashSeed());
                                              ^~
In file included from ../deps/v8/src/v8.h:8:0,
                 from /home/markus/tmp/node/out/Release/obj/gen/libraries.cc:7:
../deps/v8/include/v8.h:149:7: note: forward declaration of ‘class v8::internal::Heap’
 class Heap;
       ^~~~
In file included from ../deps/v8/src/snapshot/natives.h:8:0,
                 from /home/markus/tmp/node/out/Release/obj/gen/libraries.cc:8:
../deps/v8/src/objects.h: In member function ‘uint32_t v8::internal::HashTable<Derived, Shape, Key>::HashForObject(Key, v8::internal::Object*)’:
../deps/v8/src/objects.h:3271:55: error: invalid use of incomplete type ‘class v8::internal::Heap’
       return Shape::SeededHashForObject(key, GetHeap()->HashSeed(), object);
                                                       ^~
In file included from ../deps/v8/src/v8.h:8:0,
                 from /home/markus/tmp/node/out/Release/obj/gen/libraries.cc:7:
../deps/v8/include/v8.h:149:7: note: forward declaration of ‘class v8::internal::Heap’
 class Heap;
       ^~~~

The code is ill formed (no diagnostic required) according the C++ standard.

@targos targos added the v8 engine Issues and PRs related to the V8 dependency. label Dec 21, 2016
@targos
Copy link
Member

targos commented Dec 21, 2016

cc @nodejs/v8

@mscdex
Copy link
Contributor

mscdex commented Dec 21, 2016

@octoploid What node version?

@octoploid
Copy link
Author

Latest git branch master.

@gibfahn
Copy link
Member

gibfahn commented Dec 22, 2016

@octoploid what platform are you on?

@jeisinger
Copy link
Contributor

/cc @hashseed

@octoploid
Copy link
Author

I'm running Linux. Let me quote the C++ std:

14.6/8: "If a hypothetical instantiation of a template immediately following its definition would be ill-formed due to a construct that does not depend on a template parameter, the program is ill-formed; no diagnostic is required. If the interpretation of such a construct in the hypothetical instantiation is different from the interpretation of the corresponding construct in any actual instantiation of the template, the program is ill-formed; no diagnostic is required.
[ Note: This can happen in situations including the following: * a type used in a non-dependent name is incomplete at the point at which a template is defined but is complete at the point at which an instantiation is performed, ....

@octoploid
Copy link
Author

 % cat tc.ii
class Heap;
class A {
public:
  Heap *m_fn1();
};
template <typename> class B : A {
  void m_fn2() { m_fn1()->HashSeed; }
};
 
% g++ -c tc.ii
tc.ii: In member function ‘void B< <template-parameter-1-1> >::m_fn2()’:
tc.ii:7:25: error: invalid use of incomplete type ‘class Heap’
   void m_fn2() { m_fn1()->HashSeed; }
                         ^~
tc.ii:1:7: note: forward declaration of ‘class Heap’
 class Heap;
       ^~~~

@hashseed
Copy link
Member

it should work by including heap.h into natives.h?

@mscdex mscdex added the build Issues and PRs related to build files or the CI. label Feb 2, 2017
@sgallagher
Copy link
Contributor

For the record, just including heap.h (or heap.h plus heap-inl.h) into natives.h does not result in a successful build.

Example: https://kojipkgs.fedoraproject.org//work/tasks/8811/17888811/build.log (note: this is a transient build and will be reaped in seven days)

@kasicka
Copy link

kasicka commented Feb 22, 2017

@hashseed
Copy link
Member

Some small refactoring to move the code out of objects.h into objects-inl.h should hopefully fix this issue. I'll come up with a patch tomorrow.

@bnoordhuis
Copy link
Member

g++ 6.3.1 works okay and I'm not adventurous enough to upgrade my FC25 box to Rawhide. Any suggestions?

@sgallagher
Copy link
Contributor

g++ 6.3.1 works okay and I'm not adventurous enough to upgrade my FC25 box to Rawhide. Any suggestions?

On Fedora 25, you can do:
dnf install mock
Add your user to the mock group in /etc/group and log out and back in (so it takes effect).

Then you can do:

$ mock -r fedora-rawhide-x86_64 init
$ mock -r fedora-rawhide-x86_64 install <packages needed for building>
$ mock -r fedora-rawhide-x86_64 --copyin <src> <dest>
$ mock -r fedora-rawhide-x86_64 shell

Inside that chroot, you essentially have Fedora 26/Rawhide (running on the F25 kernel).

@kasicka
Copy link

kasicka commented Feb 28, 2017

I usually use rawhide docker images.

@bnoordhuis
Copy link
Member

mock made it real easy, thanks. The fix:

diff --git a/deps/v8/src/objects-body-descriptors.h b/deps/v8/src/objects-body-descriptors.h
index 91cb8883be..a1c3634bd7 100644
--- a/deps/v8/src/objects-body-descriptors.h
+++ b/deps/v8/src/objects-body-descriptors.h
@@ -99,7 +99,7 @@ class FixedBodyDescriptor final : public BodyDescriptorBase {
 
   template <typename StaticVisitor>
   static inline void IterateBody(HeapObject* obj, int object_size) {
-    IterateBody(obj);
+    IterateBody<StaticVisitor>(obj);
   }
 };
 

(That method can probably be removed entirely with some more effort.)

And another change to silence that very noisy forward declaration warning:

diff --git a/deps/v8/src/objects-inl.h b/deps/v8/src/objects-inl.h
index 1a8274cbf1..a237d2b753 100644
--- a/deps/v8/src/objects-inl.h
+++ b/deps/v8/src/objects-inl.h
@@ -39,6 +39,27 @@
 namespace v8 {
 namespace internal {
 
+template <typename Derived, typename Shape, typename Key>
+uint32_t HashTable<Derived, Shape, Key>::Hash(Key key) {
+  if (Shape::UsesSeed) {
+    return Shape::SeededHash(key, GetHeap()->HashSeed());
+  } else {
+    return Shape::Hash(key);
+  }
+}
+
+
+template <typename Derived, typename Shape, typename Key>
+uint32_t HashTable<Derived, Shape, Key>::HashForObject(Key key,
+                                                       Object* object) {
+  if (Shape::UsesSeed) {
+    return Shape::SeededHashForObject(key, GetHeap()->HashSeed(), object);
+  } else {
+    return Shape::HashForObject(key, object);
+  }
+}
+
+
 PropertyDetails::PropertyDetails(Smi* smi) {
   value_ = smi->value();
 }
diff --git a/deps/v8/src/objects.h b/deps/v8/src/objects.h
index 747a4f0511..b9279640e2 100644
--- a/deps/v8/src/objects.h
+++ b/deps/v8/src/objects.h
@@ -3531,22 +3531,10 @@ class HashTable : public HashTableBase {
  public:
   typedef Shape ShapeT;
 
-  // Wrapper methods
-  inline uint32_t Hash(Key key) {
-    if (Shape::UsesSeed) {
-      return Shape::SeededHash(key, GetHeap()->HashSeed());
-    } else {
-      return Shape::Hash(key);
-    }
-  }
-
-  inline uint32_t HashForObject(Key key, Object* object) {
-    if (Shape::UsesSeed) {
-      return Shape::SeededHashForObject(key, GetHeap()->HashSeed(), object);
-    } else {
-      return Shape::HashForObject(key, object);
-    }
-  }
+  // Wrapper methods.  Defined in src/objects-inl.h
+  // to break a cycle with src/heap/heap.h.
+  inline uint32_t Hash(Key key);
+  inline uint32_t HashForObject(Key key, Object* object);
 
   // Returns a new HashTable object.
   MUST_USE_RESULT static Handle<Derived> New(

We can float these as patches in v6.x but since the issue also exists in the master branch, I'll upstream them to V8.

@detailyang
Copy link
Contributor

detailyang commented Mar 14, 2017

Hello Guys.

+ 1 on Centos 6 with node https://nodejs.org/dist/v6.10.0/node-v6.10.0.tar.gz as the following:

../deps/v8/src/builtins.cc:88:68: error: invalid use of incomplete type 'class v8::internal::{anonymous}::BuiltinArguments<v8::internal::BuiltinExtraArguments:: kTarget>'
 Handle<S> BuiltinArguments<BuiltinExtraArguments::kTarget>::target() {
                                                                    ^
../deps/v8/src/builtins.cc:35:7: error: declaration of 'class v8::internal::{anonymous}::BuiltinArguments<v8::internal::BuiltinExtraArguments:: kTarget>'
 class BuiltinArguments : public Arguments {



$ /opt/gcc/bin/g++ --version
g++ (GCC) 4.8.0
Copyright (C) 2013 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ cat /etc/issue
CentOS release 6.8 (Final)
Kernel \r on an \m

Any idea?

@bnoordhuis
Copy link
Member

@detailyang Try upgrading to gcc 4.8.5 or newer. 4.8.0 has several known bugs that make it impossible to build node.

@detailyang
Copy link
Contributor

Hello.
@bnoordhuis 4.8.5 is okay for me :D
Yup, I try to build node on personal server with Centos 7 gcc 4.8.5 and It's successful. Now I'm ready to upgrade the gcc from 4.8.0 to 4.8.5 on production , you know that it's time-consuming and risky (maybe)

BTW, I think it should be mention on BUILDING.md and I will create a PR to fix this 😁

@bnoordhuis
Copy link
Member

Oh, good point. We recently discussed bumping the base line from 4.8 to 4.8.5 but looks like we didn't actually do it yet. A pull request would be welcome.

@octoploid
Copy link
Author

Any reason why this patch isn't applied? It fixes the issue for me.

@bnoordhuis
Copy link
Member

#12392 - I didn't get around to upstreaming it; things changed quite a bit upstream and I didn't have time to look into it. It's probably no longer an issue there.

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Apr 19, 2017
This is a local patch because upstream fixed it differently by moving
large chunks of code out of objects.h.  We cannot easily back-port
those changes due to their size and invasiveness.

Fixes: nodejs#10388
PR-URL: nodejs#12392
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bnoordhuis
Copy link
Member

Fixed in 2bbee49, should be in the next v7.x release.

@targos targos closed this as completed in 0ba74db Jun 23, 2017
addaleax pushed a commit that referenced this issue Jun 24, 2017
Porting #12392 to V8 5.9

Ref: #12392
Fixes: #10388
PR-URL: #13515
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
rvagg pushed a commit that referenced this issue Jun 29, 2017
Porting #12392 to V8 5.9

Ref: #12392
Fixes: #10388
PR-URL: #13515
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this issue Jul 11, 2017
This is a local patch because upstream fixed it differently by moving
large chunks of code out of objects.h.  We cannot easily back-port
those changes due to their size and invasiveness.

Fixes: #10388
PR-URL: #12392
Backport-PR-URL: #13574
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this issue Jul 11, 2017
Porting #12392 to V8 5.9

Ref: #12392
Fixes: #10388
PR-URL: #13515
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this issue Jul 18, 2017
Porting #12392 to V8 5.9

Ref: #12392
Fixes: #10388
PR-URL: #13515
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos added a commit to targos/node that referenced this issue Jul 21, 2017
addaleax pushed a commit that referenced this issue Jul 24, 2017
Porting #12392 to V8 5.9

Ref: #12392
Fixes: #10388
PR-URL: #13515
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit to MylesBorins/node that referenced this issue Aug 1, 2017
Original commit message:

    Fix GCC 7 build errors

    BUG=chromium:691681
    R=franzih@chromium.org

    Change-Id: Id7e5698487f16dc217a804f6d3f24da7213c72b9
    Reviewed-on: https://chromium-review.googlesource.com/530227
    Commit-Queue: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#46045}

PR-URL: nodejs#13517
Fixes: nodejs#10388
Refs: nodejs#12392
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit to MylesBorins/node that referenced this issue Aug 1, 2017
Original commit message:

    Fix GCC 7 build errors

    BUG=chromium:691681
    R=franzih@chromium.org

    Change-Id: Id7e5698487f16dc217a804f6d3f24da7213c72b9
    Reviewed-on: https://chromium-review.googlesource.com/530227
    Commit-Queue: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#46045}

Refs: nodejs#13517
Fixes: nodejs#10388
Refs: nodejs#12392

PR-URL: nodejs#14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this issue Aug 1, 2017
Original commit message:

    Fix GCC 7 build errors

    BUG=chromium:691681
    R=franzih@chromium.org

    Change-Id: Id7e5698487f16dc217a804f6d3f24da7213c72b9
    Reviewed-on: https://chromium-review.googlesource.com/530227
    Commit-Queue: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#46045}

Refs: #13517
Fixes: #10388
Refs: #12392

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit that referenced this issue Aug 2, 2017
Original commit message:

    Fix GCC 7 build errors

    BUG=chromium:691681
    R=franzih@chromium.org

    Change-Id: Id7e5698487f16dc217a804f6d3f24da7213c72b9
    Reviewed-on: https://chromium-review.googlesource.com/530227
    Commit-Queue: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#46045}

Refs: #13517
Fixes: #10388
Refs: #12392

Backport-PR-URL: #14574
Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-Reviewed-By: Refael Ackermann <refack@gmail.com>

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

10 participants