Skip to content

Commit

Permalink
libutil: fix args assert being thrown on Darwin in nix-eval-jobs
Browse files Browse the repository at this point in the history
This is because a dynamic_cast<nix::RootArgs *> of a (n-e-j) MyArgs
returns nullptr even though MyArgs has virtual nix::RootArgs as a
parent.

class MyArgs : virtual public nix::MixEvalArgs,
               virtual public nix::MixCommonArgs,
               virtual nix::RootArgs { ... };

So this should work right?? But it does not. We found out that it's
caused by -fvisibility=hidden in n-e-j, but honestly this code was bad
anyway.

The trivial solution is to simply stop relying on RTTI working properly
here, which is probably better OO architecture anyway. However, I am not
100% confident *this* is sound, since we have this horrible hierarchy:

      Args (defines getRoot)
     /        |           \
RootArgs  MixCommonArgs  MixEvalArgs
(overrides)

I am not confident that this is guaranteed to resolve from Args always
in the case of this override.

Assertion failed: (res), function getRoot, file src/libutil/args.cc, line 67.
6MyArgsProcess 60503 stopped
* thread NixOS#1, queue = 'com.apple.main-thread', stop reason = hit program assert
    frame NixOS#4: 0x0000000100b1a41c liblixutil.dylib`nix::Args::processArgs(std::__1::list<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>> const&, bool) [inlined] nix::Args::getRoot(this=0x00000001000d0688) at args.cc:67:5 [opt]
   64       std::cout << typeid(*p).name();
   65
   66       auto * res = dynamic_cast<RootArgs *>(p);
-> 67       assert(res);
   68       return *res;
   69   }
   70
Target 0: (nix-eval-jobs) stopped.

(lldb) p this
(MyArgs *) 0x00000001000d0688
(lldb) p *this
(nix::Args) {
  longFlags = size=180  { ... }
  shortFlags = size=4  { ... }
  expectedArgs = size=1  { ... }
  processedArgs = size=0 {}
  hiddenCategories = size=1 {
    [0] = "Options to override configuration settings"
  }
  parent = nullptr
}

We also found that if we did this:
class [[gnu::visibility("default")]] RootArgs : virtual public Args

it would work properly (???!). This is of course, very strange, because
objdump -Ct output on liblixexpr.dylib is identical both with and
without it.

Possibly related: https://www.qt.io/blog/quality-assurance/one-way-dynamic_cast-across-library-boundaries-can-fail-and-how-to-fix-it

Fixes: https://git.lix.systems/lix-project/nix-eval-jobs/issues/2
Change-Id: I6b9ed968ed56420a9c4d2dffd18999d78c2761bd
  • Loading branch information
lf- committed May 31, 2024
1 parent 0c6cb34 commit ac78c1d
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 1 deletion.
2 changes: 1 addition & 1 deletion src/libutil/args.cc
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ RootArgs & Args::getRoot()
while (p->parent)
p = p->parent;

auto * res = dynamic_cast<RootArgs *>(p);
auto res = p->asRootArgs();
assert(res);
return *res;
}
Expand Down
7 changes: 7 additions & 0 deletions src/libutil/args.hh
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,13 @@ protected:
*/
virtual void initialFlagsProcessed() {}

/**
* Returns this Args as a RootArgs if it is one, or \ref std::nullopt otherwise.
*/
virtual std::optional<std::reference_wrapper<RootArgs>> asRootArgs() {
return std::nullopt;
}

public:

void addFlag(Flag && flag);
Expand Down
4 changes: 4 additions & 0 deletions src/libutil/args/root.hh
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ protected:
*/
std::set<ExperimentalFeature> flagExperimentalFeatures;

virtual std::optional<std::reference_wrapper<RootArgs>> asRootArgs() override {
return *this;
}

private:

std::optional<std::string> needsCompletion(std::string_view s);
Expand Down

0 comments on commit ac78c1d

Please sign in to comment.