cppfront icon indicating copy to clipboard operation
cppfront copied to clipboard

[BUG] Assertion failure inside `find_definite_last_uses` in `sema.h` cppfront compiler.

Open germandiagogomez opened this issue 1 year ago • 4 comments

Describe the bug

Assertion failure inside find_definite_last_uses in sema.h cppfront compiler when using

boost::async and then continuations, not sure why exactly (could be order of evaluation when translating code? Just wild idea, not sure even if could be related).

There is a lambda created with different captures actually in each iteration of the loop, maybe that is troublesome?

To Reproduce

This piece of code seems to trigger it (go to the function the expression tasks.push_back(boost::async(:() = count_words(read_words(file$, stats*&$*, read_buf_size$))) triggers the assertion.


StringHash: @value type = {
    is_transparent : type == void;

    operator():(this, txt: * const char) = std::hash<std::string_view>()(txt);
    
    
    operator():(this, txt: std::string_view) = std::hash<std::string_view>()(txt);

    operator():(this, txt: std::string) -> std::size_t = {
        return std::hash<std::string_view>()(txt);
    }
}

OptionValueType: @union type = {
    intOpt: i32;
    stringOpt: std::string;
}

AlgorithmOptions: type == std::unordered_map<std::string, OptionValueType>;

WordsCountMap: type == phmap::parallel_flat_hash_map<std::string,
                                  std::uint64_t,
                                  StringHash,
                                  std::equal_to<>,
                                  std::allocator<std::pair<const std::string, std::uint64_t>>,
                                  7,
                                  std::mutex>;

CountWordsStats: @struct type = {
    bytes_read: std::atomic<std::int64_t> = 0;
    num_files_processed: std::atomic<std::size_t> = 0;
    average_files_size_bytes: std::atomic<std::size_t> = 0;
}

WordsViewCountMap: type == absl::flat_hash_map<std::string_view, std::uint64_t>;

WordsCountResult: type = {
    words_map: WordsCountMap;
    stats : std::unique_ptr<CountWordsStats>;

    operator=:(out this, forward wcm : WordsCountMap, 
               forward stts : std::unique_ptr<CountWordsStats>) = {
        words_map = wcm;
        stats = stts;
    }

}


count_words_with_boost_future:(rng: std::span<const std::filesystem::path>,
                     opts: AlgorithmOptions) -> WordsCountResult = {
  using namespace std;
  words_count : WordsCountMap = ();
  read_buf_size : int;
  if opts.contains("read-buffer-size") {
    read_buf_size = opts.at("read-buffer-size").intOpt();
  } else {
    read_buf_size = 4096;
  }
  words_count_mut: std::mutex = ();
  tasks: vector<boost::future<void>> = ();
  stats := new<CountWordsStats>();
  for rng do(file) {
      tasks.push_back(boost::async(:() = count_words(read_words(file$, stats*&$*, read_buf_size$)))..then(:(forward words: boost::future<WordsCountMap>) = {
              task_words := words.get();
              _ : std::lock_guard  = (words_count_mut&$*);
              for task_words do(wordtimes) {
                words_count&$*[wordtimes.first] += wordtimes.second;
              }
    }));
  }
  boost::when_all(tasks.begin(), tasks.end()).wait();
  return :WordsCountResult = (wrods_count, stats);
}

Additional context

The code has two dependencies but probably upon inspection it can be solved. Otherwise, please call me back, I will try to arrange something 100% usable without further config.

germandiagogomez avatar Aug 19 '24 12:08 germandiagogomez

Anyone could give some insights whether the compiler or myself are right about the correctness of the code? I have original C++ code that I believe to be equivalent and it works like a charm.

The problem is the lambda in the for loop fires an assert in Cpp2. Maybe @hsutter has the expertise to know whether this is a bug or not. I am not sure at all of what is going on right now.

germandiagogomez avatar Aug 24 '24 07:08 germandiagogomez

@germandiagogomez I did some debugging. It looks like the following assert is triggered:

assert(!comp || symbols[i].start);

Where comp is not null (so !comp will evaluate to false) and symbols[i].start is false.

And symbols[i] point to the compound where the open brace is at position (line:58, col:10) where:

  read_buf_size : int;
  if opts.contains("read-buffer-size") {
    read_buf_size = opts.at("read-buffer-size").intOpt();
  } else { // this is line 58
    read_buf_size = 4096;
  }

I need to dig deeper, but when I commented this assert, it worked and produced the following code:

[[nodiscard]] auto count_words_with_boost_future(cpp2::impl::in<std::span<std::filesystem::path const>> rng, 
                     cpp2::impl::in<AlgorithmOptions> opts) -> WordsCountResult{
  using namespace std;
  WordsCountMap words_count {}; 
  cpp2::impl::deferred_init<int> read_buf_size; 
  if (CPP2_UFCS(contains)(opts, "read-buffer-size")) {
    read_buf_size.construct(CPP2_UFCS(intOpt)(CPP2_UFCS(at)(opts, "read-buffer-size")));
  }else {
    read_buf_size.construct(4096);
  }
  std::mutex words_count_mut {}; 
  vector<boost::future<void>> tasks {}; 
  auto stats {cpp2_new<CountWordsStats>()}; 
  for ( auto const& file : rng ) {
      CPP2_UFCS(push_back)(tasks, boost::async([_0 = file, _1 = (&*cpp2::impl::assert_not_null(stats)), _2 = cpp2::move(read_buf_size.value())]() mutable -> void { count_words(read_words(_0, *cpp2::impl::assert_not_null(_1), _2));  }).then([_0 = (&words_count_mut), _1 = (&words_count)](auto&& words) mutable -> void
requires (std::is_same_v<boost::future<WordsCountMap>, CPP2_TYPEOF(words)>) 
      {
              auto task_words {CPP2_UFCS(get)(CPP2_FORWARD(words))}; 
                  std::lock_guard auto_1 {*cpp2::impl::assert_not_null(_0)}; 
              for ( auto const& wordtimes : cpp2::move(task_words) ) {
                CPP2_ASSERT_IN_BOUNDS((*cpp2::impl::assert_not_null(_1)), wordtimes.first) += wordtimes.second;
              }
    }));
  }
  CPP2_UFCS(wait)(boost::when_all(CPP2_UFCS(begin)(tasks), CPP2_UFCS(end)(cpp2::move(tasks))));
  return WordsCountResult{wrods_count, cpp2::move(stats)}; 
}

filipsajdak avatar Aug 24 '24 20:08 filipsajdak

@filipsajdak I had a C++ version of this working (this is a port to try Cpp2) and it did work. The code generated by Cpp2 compiler seems to be equivalent to what I wrote in C++, so I assume it should work and be correct, unless I have some hidden bug in the C++ version that never showed up :)

The same assert triggered for me BTW.

germandiagogomez avatar Aug 24 '24 21:08 germandiagogomez

I will dig more into that issue.

filipsajdak avatar Aug 24 '24 21:08 filipsajdak