[PATCH] D130072: [BOLT] Support split landing pad for stripped binaries

[Rafael Auler via Phabricator via llvm-commits]

rafauler added a comment.

Excellent work Huan! I have a few design suggestions below. Let me know what do you think.



================
Comment at: bolt/lib/Core/Exceptions.cpp:102
 void BinaryFunction::parseLSDA(ArrayRef<uint8_t> LSDASectionData,
-                               uint64_t LSDASectionAddress) {
+                               uint64_t LSDASectionAddress, bool CheckOnly) {
   assert(CurrentState == State::Disassembled && "unexpected function state");
----------------
I'm not a fan of using boolean input variables  (there are several texts written about this anti-pattern, this is just one of them: https://understandlegacycode.com/blog/what-is-wrong-with-boolean-parameters/ )

We have quite a few of these in our codebase already and it makes me uneasy. Here, it just makes code harder to read and understand. This is already a reasonably complex parser, and we should at least try to make it simpler to read. I would prefer if this function was refactored to reflect the fact that it now has two modes of operation.

If we look at parseLSDA as it is in this patch, it can be refactored using a pattern of parser logic separated from callback action. this is similar to semantic actions in a compiler parser, in which we completely separate parser from the component responsible for taking actions depending on what the parser reports as the rule being currently consumed.

put all of the current parser logic into parseLSDA, make parseLSDA receive a callback as an argument that is what is going to happen to each parsed element, and then implement checkonly=true and checkonly=false logic in the callback.

something along these lines  (let me know if it is not feasible to implement this pattern or if it is too cumbersome):

  BinaryFunction {
  private:
    parseAndWalkOverLSDA(data, Callback)
  public:
    consumeLSDAForFragmentRelationships();
    consumeLSDAForEHInfo()
  }

  parseAndWalkOverLSDA{
    parsing logic...
    for each iteration of loop {
      parsing logic
      callback()
    }
  }

  consumeLSDAForFragmentRelationships{
    auto Callback = [&] {
      establish fragment relationships logic...
    }

    parseAndWalkOverLSDA(Callback);
  }

  consumeLSDAForEHInfo {
    auto Callback = [&] {
      modify instructions to add all EH info stuff..
    }
    parseAndWalkOverLSDA(Callback);
  }


================
Comment at: bolt/lib/Core/Exceptions.cpp:543
 
-bool CFIReaderWriter::fillCFIInfoFor(BinaryFunction &Function) const {
+void CFIReaderWriter::extractLSDAAddress(BinaryFunction &Function) const {
   uint64_t Address = Function.getAddress();
----------------
fillLSDAAddressFor


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130072/new/

https://reviews.llvm.org/D130072