This is an archive of the discontinued LLVM Phabricator instance.

[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

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
789

Should it be error instead ?

2015

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 ?

2027

I think you do not need Tok:

if (!readInteger(next(), Origin))
2039

Same here.

ELF/LinkerScript.h
193

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
2015

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
789

I think so. Thanks.

2027

You're right. Will fix.

2039

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

Comment.

525

Comment.

534

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.)

543

We don't usually use auto in this context.

781–783

Revert this style change.

2017

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

ELF/LinkerScript.h
192

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

201

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

237

Needs a comment.

meadori added inline comments.Jan 21 2017, 4:46 PM
ELF/LinkerScript.cpp
412

Will fix.

525

Will fix.

534

Will fix.

543

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);
781–783

Will fix.

2017

Will fix.

ELF/LinkerScript.h
192

Will fix.

201

Will fix.

237

Will fix.

ruiu added inline comments.Jan 21 2017, 4:51 PM
ELF/LinkerScript.cpp
543

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
2060

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};
}
2083

Looks you do not really need that break.

meadori added inline comments.Jan 23 2017, 8:22 AM
ELF/LinkerScript.cpp
543

Sure, will do.

2060

I do find that more readable. Thanks.

2083

You are right. Thanks!

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

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

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

549

Return &MR here and remove MatchingMemRegion

594

Why do you have to clear this?

2047

You can change this to

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

can't you?

2053

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

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

197–198

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

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

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

Will fix.

594

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

2047

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

2053

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

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

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

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

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

2072

s/auto/char/

ELF/LinkerScript.h
197–198

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

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

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

Looks nice.

ELF/LinkerScript.h
197–198

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
541–542

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

552

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

ELF/LinkerScript.h
199

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
552

Will fix.

ELF/LinkerScript.h
199

SGTM. I will change it.

This revision was automatically updated to reflect the committed changes.