Page MenuHomePhabricator

[LinkerScript] Implement `MEMORY` command
ClosedPublic

Authored by meadori on Jan 19 2017, 12:14 PM.

Details

Summary

Hi All,

This is a work in progress patch for implementing the MEMORY command as specified here:

There are two deviations from what is specified for GNU ld:

  1. Only integer constants and *not* constant expressions are allowed in LENGTH and ORIGIN initializations.
  2. The I and L attributes are *not* implemented.

With (1) there is currently no easy way to evaluate integer
only constant expressions. This can be enhanced in the
future.

With (2) it isn't clear how these flags map to the SHF_*
flags or if they even make sense for an ELF linker.

Diff Detail

Repository
rL LLVM

Event Timeline

meadori created this revision.Jan 19 2017, 12:14 PM
ruiu edited edge metadata.Jan 19 2017, 12:40 PM

This generally seems good. One thing I can say is it needs more comments to describe the code for the first-time readers.

grimar added a subscriber: grimar.Jan 20 2017, 1:37 AM

Few comments.

ELF/LinkerScript.cpp
743 ↗(On Diff #85004)

Should it be error instead ?

1988 ↗(On Diff #85004)

You do not use AttrStr, so you can just use 'next()'.
May be worth to emit warning/error here about that attributes are not yet supported ?

2000 ↗(On Diff #85004)

I think you do not need Tok:

if (!readInteger(next(), Origin))
2012 ↗(On Diff #85004)

Same here.

ELF/LinkerScript.h
193 ↗(On Diff #85004)

You do not need initialize Name I think, it is empty by default.

grimar added inline comments.Jan 20 2017, 1:40 AM
ELF/LinkerScript.cpp
1988 ↗(On Diff #85004)

Ah you're still working on attributes, please ignore this comment then.

meadori added inline comments.Jan 21 2017, 10:52 AM
ELF/LinkerScript.cpp
743 ↗(On Diff #85004)

I think so. Thanks.

2000 ↗(On Diff #85004)

You're right. Will fix.

2012 ↗(On Diff #85004)

Will fix.

meadori updated this revision to Diff 85244.Jan 21 2017, 11:10 AM
meadori retitled this revision from [LinkerScript] WIP: Implement `MEMORY` command to [LinkerScript] Implement `MEMORY` command.
meadori edited the summary of this revision. (Show Details)
  1. Finished attribute implementation.
  1. Added tests.
ruiu added inline comments.Jan 21 2017, 2:43 PM
ELF/LinkerScript.cpp
412 ↗(On Diff #85244)

Comment.

523 ↗(On Diff #85244)

Comment.

532 ↗(On Diff #85244)

I don't think we are using `' to quote a word. I believe we write '' instead. (So please make it look similar to other code.)

541 ↗(On Diff #85244)

We don't usually use auto in this context.

776–778 ↗(On Diff #85244)

Revert this style change.

2007 ↗(On Diff #85244)

Factor out the body of this if to a separate function which returns std::pair<uint32_t, uint32_t>.

ELF/LinkerScript.h
192 ↗(On Diff #85244)

As I said in the previous comment, this needs a comment.

201 ↗(On Diff #85244)

We do not write multiple definitions in one one like that. If in doubt, please look at other code and follow the convention.

233 ↗(On Diff #85244)

Needs a comment.

meadori added inline comments.Jan 21 2017, 4:46 PM
ELF/LinkerScript.cpp
412 ↗(On Diff #85244)

Will fix.

523 ↗(On Diff #85244)

Will fix.

532 ↗(On Diff #85244)

Will fix.

541 ↗(On Diff #85244)

This is iterating a map, thus the direct declaration would be something gross like llvm::DenseMap<llvm::StringRef, MemoryRegion>::value_type. Personally, I think the auto is cleaner.

Also, there is precedence in the existing code with things like:

for (auto I = Cmd->Commands.begin(); I != E; ++I)
    process(**I);
776–778 ↗(On Diff #85244)

Will fix.

2007 ↗(On Diff #85244)

Will fix.

ELF/LinkerScript.h
192 ↗(On Diff #85244)

Will fix.

201 ↗(On Diff #85244)

Will fix.

233 ↗(On Diff #85244)

Will fix.

ruiu added inline comments.Jan 21 2017, 4:51 PM
ELF/LinkerScript.cpp
541 ↗(On Diff #85244)

Got it. Then can you use a temporary variable like this so that the type becomes obvious?

MemroyRegion &M = MR.second;
meadori updated this revision to Diff 85254.Jan 21 2017, 5:08 PM

Address review feedback concerning code style and comments.

grimar added inline comments.Jan 23 2017, 1:39 AM
ELF/LinkerScript.cpp
2055 ↗(On Diff #85254)

I would probably split this into separate function and rewrite to something like:

static uint32_t convertToFlags(char Attr) {
  if (Attr == 'w' || Attr == 'W')
    return SHF_WRITE;
  if (Attr == 'x' || Attr == 'X')
    return SHF_EXECINSTR;
  if (Attr == 'a' || Attr == 'A')
    return SHF_ALLOC;
  if (Attr != 'r' && Attr != 'R')
    setError("invalid memory region attribute: " + Attr);
return 0;
} 

std::pair<uint32_t, uint32_t> ScriptParser::readMemoryAttributes() {
  uint32_t Flags = 0;
  uint32_t NotFlags = 0;
  bool Invert = false;
  for (auto C : next()) {
    if (C == '!)
      Invert = !Invert;
    else if (Invert)
      NotFlags |= convertToFlags(C);
    else
      Flags |= convertToFlags(C);
  }
  return {Flags, NotFlags};
}
2078 ↗(On Diff #85254)

Looks you do not really need that break.

meadori added inline comments.Jan 23 2017, 8:22 AM
ELF/LinkerScript.cpp
2055 ↗(On Diff #85254)

I do find that more readable. Thanks.

2078 ↗(On Diff #85254)

You are right. Thanks!

541 ↗(On Diff #85244)

Sure, will do.

meadori updated this revision to Diff 85406.Jan 23 2017, 8:56 AM

Address more style comments from review feedback.

ruiu added inline comments.Jan 23 2017, 12:59 PM
ELF/LinkerScript.cpp
419–422 ↗(On Diff #85406)

You don't want to call error twice for one error. You can print out a multi-line error just by adding "\n" in middle of a message.

541–542 ↗(On Diff #85406)

Do you need this? A nullptr will naturally be returned without this, I think.

549 ↗(On Diff #85406)

Return &MR here and remove MatchingMemRegion

594 ↗(On Diff #85406)

Why do you have to clear this?

2047 ↗(On Diff #85406)

You can change this to

Opt.MemoryRegions[Name] = {Name, Origin, Length, Flags, NotFlags};

can't you?

2053 ↗(On Diff #85406)

You can make this a static non-member function (which is generally preferred because it makes clear that the function does not depend on any class.)

ELF/LinkerScript.h
196 ↗(On Diff #85406)

Instead of defining this constructor, can you set default values by adding = 0 to all these fields?

197–198 ↗(On Diff #85406)

I think you can remove this constructor if you initialize a struct using {} as I mentioned below.

meadori added inline comments.Jan 23 2017, 2:28 PM
ELF/LinkerScript.cpp
419–422 ↗(On Diff #85406)

I originally did that, but the formatting looks bad:

./bin/ld.lld: error: section '.data' will not fit in region 'ram'
region 'ram' overflowed by 3084 bytes

Personally I think the following is easier to understand:

./bin/ld.lld: error: section '.data' will not fit in region 'ram'
./bin/ld.lld: error: region 'ram' overflowed by 3084 bytes
541–542 ↗(On Diff #85406)

A nullptr will be returned, but the error at the bottom of the function might be triggered as well. To avoid (1) executing nop-code on an empty region map and (2) to avoid hitting that error path I chose to do an early return.

549 ↗(On Diff #85406)

Will fix.

594 ↗(On Diff #85406)

I don't, that was a left over from a previous revision. Thank you for catching it.

2047 ↗(On Diff #85406)

I can't b/c of reasons cited in previous comments.

2053 ↗(On Diff #85406)

I can't call setError if I do that. I think it is better to just inline that code back into readMemoryAttributes.

ELF/LinkerScript.h
196 ↗(On Diff #85406)

I can't b/c of the error I describe in the following comment. Since I can't remove the second constructor I have to have a default one (otherwise the compiler doesn't provide one for me).

197–198 ↗(On Diff #85406)

That doesn't work. I get errors like:

error: no match for ‘operator=’ (operand types are ‘lld::elf::MemoryRegion’ and ‘<brace-enclosed initializer list>’)
ruiu added inline comments.Jan 23 2017, 2:44 PM
ELF/LinkerScript.cpp
419–422 ↗(On Diff #85406)

Semantically I want to emit one error: for each error. I don't think this needs to be in two lines -- how about concatenating these messages with ": " instead of "\n"?

2023 ↗(On Diff #85406)

You want to return from the function when you find an error in this function to prevent cascading error messages.

2072 ↗(On Diff #85406)

s/auto/char/

ELF/LinkerScript.h
197–198 ↗(On Diff #85406)

Sorry, the number of initializers did not match with the number of members. Does this work?

{Name, Origin, Length, Origin, Flags, NotFlags}
meadori added inline comments.Jan 23 2017, 4:58 PM
ELF/LinkerScript.cpp
419–422 ↗(On Diff #85406)

That works. It looks like this now:

error: section '.data' will not fit in region 'ram': overflowed by 3084 bytes
ELF/LinkerScript.h
197–198 ↗(On Diff #85406)

I accounted for that. It seems that in-class field initialization and braced initializer lists are not compatible:

$ c++ --version
c++ (Ubuntu 4.8.4-2ubuntu1~14.04.3) 4.8.4
Copyright (C) 2013 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

meadori@build1-trusty-cs:/scratch/meadori/llvm-mainline/obj/llvm$ cat -b test.cpp
     1  struct T {
     2    unsigned x = 0;
     3  };
     4  int main(void) {
     5    T t = {1};
     6    return 0;
     7  }
meadori@build1-trusty-cs:/scratch/meadori/llvm-mainline/obj/llvm$ c++ -std=c++11 test.cpp
test.cpp: In function ‘int main()’:
test.cpp:5:11: error: could not convert ‘{1}’ from ‘<brace-enclosed initializer list>’ to ‘T’
   T t = {1};
           ^

If I remove the in-class initializers, it works.

ruiu added inline comments.Jan 23 2017, 4:59 PM
ELF/LinkerScript.cpp
419–422 ↗(On Diff #85406)

Looks nice.

ELF/LinkerScript.h
197–198 ↗(On Diff #85406)

Sure. Then I think you can remove the C++11-style intializers. I think we don't need it.

meadori updated this revision to Diff 85496.Jan 23 2017, 5:14 PM
  1. Shorten overflow error message.
  2. Get rid of MemoryRegion constructors.
  3. A few more style changes.
ruiu accepted this revision.Jan 23 2017, 5:23 PM

LGTM with a few nits.

Please verify that your new tests pass with BFD linker so that LLD's behavior is the same as BFD.

ELF/LinkerScript.cpp
551 ↗(On Diff #85496)

You can remove != 0 because any if (X) is equivalent to if ((X) != 0).

541–542 ↗(On Diff #85406)

Then can you move this piece of code to the beginning of this function to return earlier?

ELF/LinkerScript.h
199 ↗(On Diff #85496)

I found that Index is a bit confusing. Maybe Offset?

This revision is now accepted and ready to land.Jan 23 2017, 5:23 PM

I will fix the remaining nits and double-check with gold\ld. Thanks for all the review!

ELF/LinkerScript.cpp
551 ↗(On Diff #85496)

Will fix.

ELF/LinkerScript.h
199 ↗(On Diff #85496)

SGTM. I will change it.

This revision was automatically updated to reflect the committed changes.