Page MenuHomePhabricator

[ELF] - Introduce helper for iterating over linker commands.
AbandonedPublic

Authored by grimar on Apr 2 2018, 8:38 AM.

Details

Summary

We often need to iterate over linker script commands.
And very often we need only a single command type in a loop.
I suggest adding the helper for convenience.
The patch shows the change I am suggesting.

Diff Detail

Event Timeline

grimar created this revision.Apr 2 2018, 8:38 AM
arichardson added inline comments.Apr 2 2018, 9:41 AM
ELF/LinkerScript.h
319

Since this only seems to be used in foreach loops maybe using llvm::make_filter_range instead of creating a new vector would make sense?

ruiu added a comment.Apr 2 2018, 10:44 AM

I'm personally not too excited about this kind of higher-order function, and my first impression is that the original code is better. I might be convinced either way, but I think I'm mildly against doing this.

I like this, but I agree that we should avoid allocating a temporary std::vector.

grimar updated this revision to Diff 140810.Apr 3 2018, 9:19 AM
  • Reimplemented with use of make_filter_range.

(honestly, this looking a bit scary for me, though I still like the caller side simplifications).

grimar added a comment.Apr 3 2018, 9:24 AM

I'm personally not too excited about this kind of higher-order function, and my first impression is that the original code is better. I might be convinced either way, but I think I'm mildly against doing this.

The original code is sometimes hard to read IMO.
For example constructions like below are simplified greatly.

for (BaseCommand *Base : SectionCommands) {
auto *Sec = dyn_cast<OutputSection>(Base);
if (!Sec)
 continue;

You do not need to think about what is Base2 anymore:

for (BaseCommand *Base2 : Sec->SectionCommands)
if (auto *Cmd = dyn_cast<SymbolAssignment>(Base2))
  declareSymbol(Cmd);
grimar updated this revision to Diff 140814.Apr 3 2018, 9:39 AM
  • Cleanup.
espindola added inline comments.Apr 3 2018, 2:05 PM
ELF/LinkerScript.h
319

s/static/inline, no?

Using auto as a return type is c++14 according to clang.

This could probably use a map + filter:

template <class CmdType>
using Foo = llvm::filter_iterator<ArrayRef<BaseCommand *>::iterator,
                                decltype(&dynCastToCmdType<CmdType>)>;

template <class CmdType>
using Bar =
  llvm::mapped_iterator<Foo<CmdType>, decltype(dynCastToCmdType<CmdType>) *>;

template <class CmdType>
llvm::iterator_range<Bar<CmdType>> filter(ArrayRef<BaseCommand *> V) {

But it feels like some of this code should be in LLVM. In particular, LLVM should probably have

  • a make_mapped_range function
  • type alias for filter_iterator_range

The code I have is:

template <class CmdType> CmdType *castToCmdType(BaseCommand *Cmd) {
  return cast<CmdType>(Cmd);
}
template <class CmdType> bool isaCmdType(const BaseCommand *Cmd) {
  return isa<CmdType>(Cmd);
}
template <class CmdType>
using Foo = llvm::filter_iterator<ArrayRef<BaseCommand *>::iterator,
                                  decltype(&isaCmdType<CmdType>)>;
template <class CmdType>
using Bar =
    llvm::mapped_iterator<Foo<CmdType>, decltype(castToCmdType<CmdType>) *>;
template <class CmdType>
llvm::iterator_range<Bar<CmdType>> filter(ArrayRef<BaseCommand *> V) {
  auto R = llvm::make_filter_range(V, isaCmdType<CmdType>);
  // FIXME: llvm should have a make_mapped_range
  auto B = map_iterator(R.begin(), castToCmdType<CmdType>);
  auto E = map_iterator(R.end(), castToCmdType<CmdType>);
  return llvm::make_range(B, E);
}
grimar updated this revision to Diff 141854.Apr 10 2018, 8:02 AM
  • Used the approach suggested.
grimar added inline comments.Apr 10 2018, 8:09 AM
ELF/LinkerScript.h
319

Using auto as a return type is c++14 according to clang.

I thought we can use c++14 features as long as minimal versions of clang/gcc version accept it.
Not 100% sure that is the case for auto though honestly.

espindola accepted this revision.Apr 10 2018, 2:55 PM

LGTM

ELF/LinkerScript.h
319

Good question. It might be a good idea to ask on llvm-dev. At least the clang build is still using -std=c++11.

This revision is now accepted and ready to land.Apr 10 2018, 2:55 PM
grimar updated this revision to Diff 141961.Apr 11 2018, 2:09 AM
  • Used filter<> in a few more places.

Rui, are you OK to land this version? It reduces nesting and simplifies loops..

arichardson added inline comments.Apr 11 2018, 3:29 AM
ELF/LinkerScript.h
320

This wrapper function could also be replaced with

template<typename CmdType>
using CastCmdType = CmdType *(*)(BaseCommand*);

And then using static_cast<CastCmdType<CmdType>>(&llvm::cast<CmdType, BaseCommand>) instead of castToCmdType<CmdType>.

However, I'm not sure that's any easier to understand.

grimar abandoned this revision.May 15 2018, 7:42 AM