[ELF] Support PROVIDE and PROVIDE_HIDDEN within SECTIONS {} block

[evgeny777]

Summary

Basic support for PROVIDE and PROVIDE_HIDDEN, currently only within SECTIONS

The existing parser does not validate expressions. To support PROVIDE(sym = expr), I implemented simple simple correctness validation based on brace counter. It aborts
reading expression tokens, if extra ')' is seen. This changed error message in one of the tests

[ruiu]

ELF/LinkerScript.cpp

281–282

Why did you remove {}?

459

This doesn't look beautiful. Isn't there any better way?

725

Let's name this readProvide as Body is used often to mean a symbol.

729

Create doesn't feel like a good name. How about Assign?

803–811

Is this change related to the feature you are adding in this patch?

ELF/LinkerScript.h

60

Please define this enum in SymbolAssignment class (as a non-enum class).

63

I wouldn't use a default argument unless it really improves to readability.

[evgeny777]

ELF/LinkerScript.cpp

459

I need some flag, so that I don't override value of provided symbol later in assignAddresses, in case it is already defined somewhere. I'll think how to rewrite it, and open to suggestions.

803–811

Yes. As I said in summary, this is intended to early terminate the expression reader in case we see extra ')' bracket. Otherwise parser will eat tokens till it finds semicolon

[grimar]

ELF/LinkerScript.cpp

803–811

This should be fixed in separate patch.

test/ELF/linkerscript-symbols.s

43

somesym = 0
(just hard to read)

[ruiu]

ELF/LinkerScript.cpp

459

How about adding a SymbolBody* to SymbolAssignment class and set it to the return value of addAbsolute?

[evgeny777]

ELF/LinkerScript.cpp

459

The problem is that current version overrides symbol value in case it is already defined (addAbsolute is not called) and is not assigned inside PROVIDE() or PROVIDE_HIDDEN(). So in case you specify "end = ." in script, the "end" symbol will be overridden, no matter has it been defined or not.

This was done in one of my previous patches in order to be able to override reserved symbols (end, etext, edata) in linker script, because currently they are always created in addReservedSymbols(). May be this should be fixed first, I don't know.

803–811

May be, but actually this parser improvement is only needed for PROVIDE(). I don't mind making separate review, but not sure if it's really needed. Rui, what's your opinion?

[grimar]

ELF/LinkerScript.cpp

803–811

Point here that this feature is useful itself and also should have a testcase which is unrelative to what this patch do. That not only make this patch a bit smaller, but also leaves this improvement in if patch be reverted, for example.

[ruiu]

ELF/LinkerScript.cpp

283

So, is it equivalent to Cmd->Provision == SymbolProvision::Create, no?

803–811

I don't think we even want it. If there's an error, it'll be found out, right?

[evgeny777]

ELF/LinkerScript.cpp

283

Not exactly. It could also be SymbolProvision::Provide and SymbolProvision::ProvideHidden.

We set Provision to Ignore only when the following two conditions are met:
a) Initially Provision == Provide or Provsion == ProvideHidden
b) Symbol has been already defined (Symtab::find(Name) returns valid pointer)

In all other cases we should set the Symbol value

[ruiu]

I believe there's a better way, but it's probably too detailed and doesn't have to be done in this patch. But at least can you separate add a separate flag to record whether a symbol was present or not, so that we don't mutate parse results in an obscure way?

Also I believe you want to define two boolean members, Provide and Hidden, instead of one member that has a combination of the two variables.

[evgeny777]

Review updated

[ruiu]

ELF/LinkerScript.cpp

450–451

Negate the condition to reduce the indentation level.

auto *Cmd = dyn_cast<...>(...);
if (!Cmd || Cmd->Name == ".")
  continue;

795

When you add { please add {} to the previous if (or else) clauses, so that they look the same.

803–811

Can you remove it?

[evgeny777]

ELF/LinkerScript.cpp

803–811

What exactly? The "if (Tok == ";")" ? If so, how will we handle normal symbol assignments then? Like "mysym = 0;"

[ruiu]

ELF/LinkerScript.cpp

803–811

I meant the new code you added to this function in this patch.

[evgeny777]

ELF/LinkerScript.cpp

803–811

The new code is used to stop reading expression in case extra ')' is seen in expression.
This happens in the case below:
PROVIDE(a = 100);

If you want this code to be removed then please suggest how to handle the case above, because ')' will be read as part of expression otherwise.

[ruiu]

Essentially looks good. Please upload the latest patch with all review comments applied. Thanks.

ELF/LinkerScript.cpp

803–811

Ah, I think I finally understood why you added this code. It makes sense. But please add a function comment saying that this function reads a balanced expression until ";".

[evgeny777]

Review updated

[ruiu]

LGTM with a nit.

ELF/LinkerScript.cpp

807

This look a bit tricky. Straightforward code would be

if (Tok == "(")
  ++Braces;
else if (Tok == ")")
  --Braces;

ELF/LinkerScript.h

 enum SectionsCommandKind { AssignmentKind, OutputSectionKind };
 
 struct BaseCommand {
   BaseCommand(int K) : Kind(K) {}
   virtual ~BaseCommand() {}
   int Kind;
 };
 
 struct SymbolAssignment : BaseCommand {
   SymbolAssignment(StringRef Name, std::vector<StringRef> &Expr)
       : BaseCommand(AssignmentKind), Name(Name), Expr(std::move(Expr)) {}
   static bool classof(const BaseCommand *C);
   StringRef Name;
   std::vector<StringRef> Expr;
+  bool Provide = false;
+  // Hidden and Ignore can be true, only if Provide is true
+  bool Hidden = false;
+  bool Ignore = false;
 };
 
 struct OutputSectionCommand : BaseCommand {
   OutputSectionCommand(StringRef Name)
       : BaseCommand(OutputSectionKind), Name(Name) {}
   static bool classof(const BaseCommand *C);
   StringRef Name;
   std::vector<StringRef> Phdrs;

ELF/LinkerScript.cpp

void LinkerScript<ELFT>::assignAddresses(
   // Assign addresses as instructed by linker script SECTIONS sub-commands.
   Dot = Out<ELFT>::ElfHeader->getSize() + Out<ELFT>::ProgramHeaders->getSize();
   uintX_t MinVA = std::numeric_limits<uintX_t>::max();
   uintX_t ThreadBssOffset = 0;
 
   for (const std::unique_ptr<BaseCommand> &Base : Opt.Commands) {
     if (auto *Cmd = dyn_cast<SymbolAssignment>(Base.get())) {
       uint64_t Val = evalExpr(Cmd->Expr, Dot);
       if (Cmd->Name == ".") {
-
         Dot = Val;
-      } else {
+      } else if (!Cmd->Ignore) {
         auto *D = cast<DefinedRegular<ELFT>>(Symtab<ELFT>::X->find(Cmd->Name));
         D->Value = Val;
       }
       continue;
     }
 
     // Find all the sections with required name. There can be more than
     // one section with such name, if the alignment, flags or type
int LinkerScript<ELFT>::compareSections(StringRef A, StringRef B) {
   int I = getSectionIndex(A);
   int J = getSectionIndex(B);
   if (I == INT_MAX && J == INT_MAX)
     return 0;
   return I < J ? -1 : 1;
 }
 
 template <class ELFT> void LinkerScript<ELFT>::addScriptedSymbols() {
   for (const std::unique_ptr<BaseCommand> &Base : Opt.Commands)
     if (auto *Cmd = dyn_cast<SymbolAssignment>(Base.get()))
-      if (Cmd->Name != "." && Symtab<ELFT>::X->find(Cmd->Name) == nullptr)
-        Symtab<ELFT>::X->addAbsolute(Cmd->Name, STV_DEFAULT);
+      if (Cmd->Name != ".") {
+        if (Symtab<ELFT>::X->find(Cmd->Name) == nullptr)
+          Symtab<ELFT>::X->addAbsolute(Cmd->Name,
+                                       Cmd->Hidden ? STV_HIDDEN : STV_DEFAULT);
+        else
+          // Symbol already exists in symbol table. If it is provided
+          // then we can't override its value.
+          Cmd->Ignore = Cmd->Provide;
+      }
 }
 
 template <class ELFT> bool LinkerScript<ELFT>::hasPhdrsCommands() {
   return !Opt.PhdrsCommands.empty();
 }
 
 // Returns indices of ELF headers containing specific section, identified
 // by Name. Each index is a zero based number of ELF header listed within
private:
   void readPhdrs();
   void readSearchDir();
   void readSections();
 
   void readLocationCounterValue();
   void readOutputSectionDescription(StringRef OutSec);
   std::vector<StringRef> readOutputSectionPhdrs();
   unsigned readPhdrType();
-  void readSymbolAssignment(StringRef Name);
+  void readProvide(bool Hidden);
+  SymbolAssignment *readSymbolAssignment(StringRef Name);
   std::vector<StringRef> readSectionsCommandExpr();
 
   const static StringMap<Handler> Cmd;
   ScriptConfiguration &Opt = *ScriptConfig;
   StringSaver Saver = {ScriptConfig->Alloc};
   bool IsUnderSysroot;
 };
 
void ScriptParser::readSections() {
   expect("{");
   while (!Error && !skip("}")) {
     StringRef Tok = peek();
     if (Tok == ".") {
       readLocationCounterValue();
       continue;
     }
     next();
-    if (peek() == "=")
+    if (Tok == "PROVIDE")
+      readProvide(false);
+    else if (Tok == "PROVIDE_HIDDEN")
+      readProvide(true);
+    else if (peek() == "=")
       readSymbolAssignment(Tok);
     else
       readOutputSectionDescription(Tok);
   }
 }
 
 void ScriptParser::readLocationCounterValue() {
   expect(".");
   expect("=");
   std::vector<StringRef> Expr = readSectionsCommandExpr();
   if (Expr.empty())
     error("error in location counter expression");
   else
     Opt.Commands.push_back(llvm::make_unique<SymbolAssignment>(".", Expr));
if (!Tok.startswith("=0x")) {
       return;
     }
     Tok = Tok.substr(3);
     Cmd->Filler = parseHex(Tok);
     next();
   }
 }
 
-void ScriptParser::readSymbolAssignment(StringRef Name) {
+void ScriptParser::readProvide(bool Hidden) {
+  expect("(");
+  if (SymbolAssignment *Assignment = readSymbolAssignment(next())) {
+    Assignment->Provide = true;
+    Assignment->Hidden = Hidden;
+  }
+  expect(")");
+  expect(";");
+}
+
+SymbolAssignment *ScriptParser::readSymbolAssignment(StringRef Name) {
   expect("=");
   std::vector<StringRef> Expr = readSectionsCommandExpr();
   if (Expr.empty())
     error("error in symbol assignment expression");
-  else
+  else {
     Opt.Commands.push_back(llvm::make_unique<SymbolAssignment>(Name, Expr));
+    return static_cast<SymbolAssignment *>(Opt.Commands.back().get());
+  }
+  return nullptr;
 }
 
 std::vector<StringRef> ScriptParser::readSectionsCommandExpr() {
+  int Braces = 0;
   std::vector<StringRef> Expr;
   while (!Error) {
-    StringRef Tok = next();
+    StringRef Tok = peek();
+    Braces += (Tok == "(");
+    Braces -= (Tok == ")");
+    if (Braces < 0)
+      break;
+    next();
     if (Tok == ";")
       break;
     Expr.push_back(Tok);
   }
   return Expr;
 }
 
 std::vector<StringRef> ScriptParser::readOutputSectionPhdrs() {

test/ELF/linkerscript-locationcounter.s

 # BRACKETERR: unexpected EOF
 
 ## Missing opening bracket.
 # RUN: echo "SECTIONS { \
 # RUN:  . = 0x10000 + 0x1000 + 0x1000) * 0x2; \
 # RUN: }" > %t.script
 # RUN: not ld.lld %t --script %t.script -o %t2 2>&1 | \
 # RUN:  FileCheck --check-prefix=BRACKETERR2 %s
-# BRACKETERR2: stray token: )
+# BRACKETERR2: expected, but got *
 
 ## Empty expression.
 # RUN: echo "SECTIONS { \
 # RUN:  . = ; \
 # RUN: }" > %t.script
 # RUN: not ld.lld %t --script %t.script -o %t2 2>&1 | \
 # RUN:  FileCheck --check-prefix=ERREXPR %s
 # ERREXPR: error in location counter expression

test/ELF/linkerscript-symbols.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
 
+# Simple symbol assignment. Should raise conflict in case we 
+# have duplicates in any input section, but currently simply
+# replaces the value.
 # RUN: echo "SECTIONS {.text : {*(.text.*)} text_end = .;}" > %t.script
 # RUN: ld.lld -o %t1 --script %t.script %t
-# RUN: llvm-objdump -t %t1 | FileCheck %s
-# CHECK: 0000000000000121         *ABS*    00000000 text_end
+# RUN: llvm-objdump -t %t1 | FileCheck --check-prefix=SIMPLE %s
+# SIMPLE: 0000000000000121         *ABS*    00000000 text_end
+
+# Provide new symbol. The value should be 1, like set in PROVIDE()
+# RUN: echo "SECTIONS { PROVIDE(newsym = 1);}" > %t.script
+# RUN: ld.lld -o %t1 --script %t.script %t
+# RUN: llvm-objdump -t %t1 | FileCheck --check-prefix=PROVIDE1 %s
+# PROVIDE1: 0000000000000001         *ABS*    00000000 newsym
+
+# Provide new symbol (hidden). The value should be 1
+# RUN: echo "SECTIONS { PROVIDE_HIDDEN(newsym = 1);}" > %t.script
+# RUN: ld.lld -o %t1 --script %t.script %t
+# RUN: llvm-objdump -t %t1 | FileCheck --check-prefix=HIDDEN1 %s
+# HIDDEN1: 0000000000000001         *ABS*    00000000 .hidden newsym
+
+# Provide existing symbol. The value should be 0, even though we 
+# have value of 1 in PROVIDE()
+# RUN: echo "SECTIONS { PROVIDE(somesym = 1);}" > %t.script
+# RUN: ld.lld -o %t1 --script %t.script %t
+# RUN: llvm-objdump -t %t1 | FileCheck --check-prefix=PROVIDE2 %s
+# PROVIDE2: 0000000000000000         *ABS*    00000000 somesym
+
+# Provide existing symbol. The value should be 0, even though we 
+# have value of 1 in PROVIDE(). Visibility should not change
+# RUN: echo "SECTIONS { PROVIDE(somesym = 1);}" > %t.script
+# RUN: ld.lld -o %t1 --script %t.script %t
+# RUN: llvm-objdump -t %t1 | FileCheck --check-prefix=HIDDEN2 %s
+# HIDDEN2: 0000000000000000         *ABS*    00000000 somesym
 
 .global _start
 _start:
  nop
+
+.global somesym
+somesym = 0