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

Huan Nguyen via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jul 26 12:44:05 PDT 2022


nhuhuan added inline comments.


================
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");
----------------
rafauler wrote:
> 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);
>   }
Thanks for your suggestions, @rafaelauler.

I generally agree that using callbacks improves maintainability. However, a few things are holding me back.

(a) Callbacks may need too many arguments
I assume that we don't want to add parsing logic remained in parseAndWalkOverLSDA
If so, there are simply too many arguments for one general callback.

(b) Avoid printing duplicate debugging messages.
If printing message in callbacks, then many more arguments.
So parseAndWalkOverLSDA need to take another boolean AllowPrintingMessage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130072



More information about the llvm-commits mailing list