[LLD][ELD] - Do not reject INFO output section type when used with a start address.

[grimar]

Summary

This is https://bugs.llvm.org/show_bug.cgi?id=38625

LLD accept this:

.stack (INFO) :
{

but not this:

.stack address_expression (INFO) :
{

The patch fixes it.

[ruiu]

This function seems a bit too complicated compared to what it should do. Can you simplify if you add peek2() function that returns Tokens[Pos+2] from the lexer?

[grimar]

• Addressed review comments.

[ruiu]

ELF/ScriptLexer.cpp

248–252 ↗

(On Diff #161675)

This is perhaps fine, but why don't you just return Tokens[Pos+2]?

ELF/ScriptParser.cpp

735 ↗

(On Diff #161675)

I don't think you need a helper function. Can you do something like this?

unless the following two tokens are "(" and ("LOAD", "INFO" or ...)
  read expression
if the next token is "("
  read "LOAD", "INFO", ...

[bd1976llvm]

Hi George. Do we actually want this behaviour?

The GNU linker docs states that these output section description type attributes are: " supported for backward compatibility, and are rarely used" which to me reads like deprecated.

We could instead emit an error that these are deprecated and possibly mention which input sections would be affected.

[grimar]

In D51027#1207342, @bd1976llvm wrote:

Hi George. Do we actually want this behaviour?

The GNU linker docs states that these output section description type attributes are: " supported for backward compatibility, and are rarely used" which to me reads like deprecated.

We could instead emit an error that these are deprecated and possibly mention which input sections would be affected.

Hi Ben,

We already had a support of "INFO" (I believe we had a user request for that previously). This patch fixes https://bugs.llvm.org/show_bug.cgi?id=38625
and does not bring anything very new, but just teaches parser about one more possible case.

Also, note that we already support the following:

.stack address_expression (NOLOAD) :
{

but not the

.stack address_expression (INFO) :
{

yet. So I think it is worth to implement both for consistency and few users we have.

[bd1976llvm]

In D51027#1207356, @grimar wrote:

Also, note that we already support the following:

.stack address_expression (NOLOAD) :
{

but not the

.stack address_expression (INFO) :
{

yet. So I think it is worth to implement both for consistency and few users we have.

The difference is that NOLOAD is not a deprecated feature, and has an effect that would otherwise be difficult to achieve. INFO, OVERLAY etc.. are deprecated, and the user just needs to set section flags correctly to achieve the same effect.

[grimar]

+Peter.
A little discussion about why "COPY"/"INFO"/"OVERLAY" was implemented is here: D43071.

[peter.smith]

I'm not sure it is possible to do exactly what the author is doing with https://bugs.llvm.org/show_bug.cgi?id=38625 with a section without SHF_ALLOC. The entire contents of the output section are defined by .= stack_start. As stack_start is calculated by the linker I don't think it would have been possible to do this with a section.

My preference would be to support the INFO, COPY, OVERLAY with an address for embedded users. Many people start by porting examples from GNU ld and aren't experts in linker scripts or startup code. I think it would be useful to be as compatible as possible. I also think we can't imply deprecation from "supported for backwards compatibility" as deprecation often implies removal at a future point.

PROVIDE(_stack_start = ORIGIN(RAM) + LENGTH(RAM));
.stack _edata (INFO) :
{
  _estack = .;
  . = _stack_start;
  _sstack = .;
} > RAM

In the arm-none-eabi gcc toolchain the sample linker scripts using COPY have a .heap and .stack section to provide the size that are already not SHF_ALLOC (so the COPY isn't strictly necessary):

	.heap (COPY):
	{
		__end__ = .;
		PROVIDE(end = .);
		*(.heap*)
		__HeapLimit = .;
	} > RAM

	/* .stack_dummy section doesn't contains any symbols. It is only
	 * used for linker to calculate size of stack sections, and assign
	 * values to stack symbols later */
	.stack_dummy (COPY):
	{
		*(.stack*)
	} > RAM

With the .heap and .stack sections being defined in the startup code for a particular device with something like:

	.section .stack
	.align	3
#ifdef __STACK_SIZE
	.equ	Stack_Size, __STACK_SIZE
#else
	.equ	Stack_Size, 0xc00
#endif
	.globl	__StackTop
	.globl	__StackLimit
__StackLimit:
	.space	Stack_Size
	.size	__StackLimit, . - __StackLimit
__StackTop:
	.size	__StackTop, . - __StackTop

[bd1976llvm]

Thanks for the detailed reply Peter!

I am certainly convinced that these features are not deprecated if they feature in examples in your documentation. Also, as you point out, how can you otherwise control the output section properties for entirely artificial output sections (well apart from either binary editing after linking, or creating an empty section with the correct properties in the inputs and assigning it to the output section).

Do you have a better source of documentation for these linker script features than the gnu docs. I find the gnu docs difficult, for example the gnu docs state for NOLOAD that: "the section should be marked as not loadable, so that it will not be loaded into memory when the program is run". Which I find difficult to interpret in in terms of ELF and leaves me required to read the source code to understand what NOLOAD does!

[peter.smith]

Do you have a better source of documentation for these linker script features than the gnu docs. I find the gnu docs difficult, for example the gnu docs state for NOLOAD that: "the section should be marked as not loadable, so that it will not be loaded into memory when the program is run". Which I find difficult to interpret in in terms of ELF and leaves me required to read the source code to understand what NOLOAD does!

I really wish I did; most of the time it is having to go to the source code, binutils mailing list posts or commit logs, to work out what is supposed to happen.

[grimar]

Thanks, Peter!

• Addressed Rui's comments.

ELF/ScriptLexer.cpp

248–252 ↗

(On Diff #161675)

Tried to be consistent with peek() implementation. Fixed.

ELF/ScriptParser.cpp

735 ↗

(On Diff #161675)

Done.

[grimar]

Ping.

[ruiu]

LGTM

lld/trunk/ELF/ScriptLexer.h

public:
   explicit ScriptLexer(MemoryBufferRef MB);
 
   void setError(const Twine &Msg);
   void tokenize(MemoryBufferRef MB);
   static StringRef skipSpace(StringRef S);
   bool atEOF();
   StringRef next();
   StringRef peek();
+  StringRef peek2();
   void skip();
   bool consume(StringRef Tok);
   void expect(StringRef Expect);
   bool consumeLabel(StringRef Tok);
   std::string getCurrentLocation();
 
   std::vector<MemoryBufferRef> MBs;
   std::vector<StringRef> Tokens;

lld/trunk/ELF/ScriptLexer.cpp

 StringRef ScriptLexer::peek() {
   StringRef Tok = next();
   if (errorCount())
     return "";
   Pos = Pos - 1;
   return Tok;
 }
 
+StringRef ScriptLexer::peek2() {
+  skip();
+  StringRef Tok = next();
+  if (errorCount())
+    return "";
+  Pos = Pos - 2;
+  return Tok;
+}
+
 bool ScriptLexer::consume(StringRef Tok) {
   if (peek() == Tok) {
     skip();
     return true;
   }
   return false;
 }
 

lld/trunk/ELF/ScriptParser.cpp

private:
   void readTarget();
   void readVersion();
   void readVersionScriptCommand();
 
   SymbolAssignment *readSymbolAssignment(StringRef Name);
   ByteCommand *readByteCommand(StringRef Tok);
   uint32_t readFill();
   uint32_t parseFill(StringRef Tok);
+  bool readSectionDirective(OutputSection *Cmd, StringRef Tok1, StringRef Tok2);
   void readSectionAddressType(OutputSection *Cmd);
   OutputSection *readOverlaySectionDescription();
   OutputSection *readOutputSectionDescription(StringRef OutSec);
   std::vector<BaseCommand *> readOverlay();
   std::vector<StringRef> readOutputSectionPhdrs();
   InputSectionDescription *readInputSectionDescription(StringRef Tok);
   StringMatcher readFilePatterns();
   std::vector<SectionPattern> readInputSectionsList();

 // https://sourceware.org/binutils/docs/ld/Output-Section-Data.html
 uint32_t ScriptParser::readFill() {
   expect("(");
   uint32_t V = parseFill(next());
   expect(")");
   return V;
 }
 
+// Tries to read the special directive for an output section definition which
+// can be one of following: "(NOLOAD)", "(COPY)", "(INFO)" or "(OVERLAY)".
+// Tok1 and Tok2 are next 2 tokens peeked. See comment for readSectionAddressType below.
+bool ScriptParser::readSectionDirective(OutputSection *Cmd, StringRef Tok1, StringRef Tok2) {
+  if (Tok1 != "(")
+    return false;
+  if (Tok2 != "NOLOAD" && Tok2 != "COPY" && Tok2 != "INFO" && Tok2 != "OVERLAY")
+    return false;
+
+  expect("(");
+  if (consume("NOLOAD")) {
+    Cmd->Noload = true;
+  } else {
+    skip(); // This is "COPY", "INFO" or "OVERLAY".
+    Cmd->NonAlloc = true;
+  }
+  expect(")");
+  return true;
+}
+
 // Reads an expression and/or the special directive for an output
 // section definition. Directive is one of following: "(NOLOAD)",
 // "(COPY)", "(INFO)" or "(OVERLAY)".
 //
 // An output section name can be followed by an address expression
 // and/or directive. This grammar is not LL(1) because "(" can be
 // interpreted as either the beginning of some expression or beginning
 // of directive.
 //
 // https://sourceware.org/binutils/docs/ld/Output-Section-Address.html
 // https://sourceware.org/binutils/docs/ld/Output-Section-Type.html
 void ScriptParser::readSectionAddressType(OutputSection *Cmd) {
-  if (consume("(")) {
-    if (consume("NOLOAD")) {
-      expect(")");
-      Cmd->Noload = true;
-      return;
-    }
-    if (consume("COPY") || consume("INFO") || consume("OVERLAY")) {
-      expect(")");
-      Cmd->NonAlloc = true;
-      return;
-    }
-    Cmd->AddrExpr = readExpr();
-    expect(")");
-  } else {
-    Cmd->AddrExpr = readExpr();
-  }
+  if (readSectionDirective(Cmd, peek(), peek2()))
 
-  if (consume("(")) {
-    expect("NOLOAD");
-    expect(")");
-    Cmd->Noload = true;
-  }
+  Cmd->AddrExpr = readExpr();
+  if (peek() == "(" && !readSectionDirective(Cmd, "(", peek2()))
+    setError("unknown section directive: " + peek2());
 }
 
 static Expr checkAlignment(Expr E, std::string &Loc) {
   return [=] {
     uint64_t Alignment = std::max((uint64_t)1, E().getValue());
     if (!isPowerOf2_64(Alignment)) {
       error(Loc + ": alignment must be power of 2");
       return (uint64_t)1; // Return a dummy value.

lld/trunk/test/ELF/linkerscript/info-section-type.s

 # RUN: echo "SECTIONS { .bar (INFO) : { *(.foo) } };" > %t.script
 # RUN: ld.lld -o %t --script %t.script %t.o
 # RUN: llvm-readobj -sections %t | FileCheck %s --check-prefix=NONALLOC
 
 # RUN: echo "SECTIONS { .bar (OVERLAY) : { *(.foo) } };" > %t.script
 # RUN: ld.lld -o %t --script %t.script %t.o
 # RUN: llvm-readobj -sections %t | FileCheck %s --check-prefix=NONALLOC
 
+# RUN: echo "SECTIONS { .bar 0x20000 (INFO) : { *(.foo) } };" > %t.script
+# RUN: ld.lld -o %t --script %t.script %t.o
+# RUN: llvm-readobj -sections %t | FileCheck %s --check-prefix=NONALLOC
+
+# RUN: echo "SECTIONS { .bar 0x20000 (BAR) : { *(.foo) } };" > %t.script
+# RUN: not ld.lld -o %t --script %t.script %t.o 2>&1 |\
+# RUN:   FileCheck %s --check-prefix=UNKNOWN
+# UNKNOWN: unknown section directive: BAR
+
 .section .foo,"a",@progbits
 .zero 1