This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Linkerscript: support VERSION command.
AbandonedPublic

Authored by grimar on Aug 17 2016, 7:00 AM.

Details

Reviewers
ruiu
rafael
Summary

From PR28926 description:

Symbol version information can be provided via the --version-script command line option, or can be embedded into a VERSION block in a linker script:

VERSION { version-script-commands }

FreeBSD uses this for the Linux ABI vDSO support, presumably from the same use in the Linux kernel.

Reference: https://www.sourceware.org/binutils/docs/ld/VERSION.html#VERSION

Patch implements this feature.

Diff Detail

Event Timeline

grimar updated this revision to Diff 68345.Aug 17 2016, 7:00 AM
grimar retitled this revision from to [ELF] - Linkerscript: support VERSION command..
grimar updated this object.
grimar added a reviewer: ruiu.
grimar added subscribers: llvm-commits, grimar, evgeny777, davide.
ruiu edited edge metadata.Aug 17 2016, 12:15 PM

I don't see a reason to keep LinkerScript and SymbolListFile being separated. I'd think we should move code from SymbolListFile to LinkerScript.

In D23609#518431, @ruiu wrote:

I don't see a reason to keep LinkerScript and SymbolListFile being separated. I'd think we should move code from SymbolListFile to LinkerScript.

D23660 do that change.

grimar updated this revision to Diff 69842.Aug 31 2016, 6:41 AM
grimar added a reviewer: rafael.
  • Reimplemented to use head changes.
ruiu added inline comments.Aug 31 2016, 7:14 AM
ELF/LinkerScript.cpp
716–719

Don't check this error inside this function. Instead, check this error outside of this function. IsCommand should be removed.

grimar updated this revision to Diff 69855.Aug 31 2016, 8:14 AM
  • Addressed review comments.
ruiu added a comment.Aug 31 2016, 8:44 AM

What I was trying to say is to remove IsCommand parameter and do not rely on return value to determine what we are parsing. We should know what we are doing without that value. If you need to read X and Y, define readX and readY, instead of readZ(bool IsX) or something like that.

grimar updated this revision to Diff 69870.Aug 31 2016, 9:49 AM
  • Addressed review comments.
In D23609#530418, @ruiu wrote:

What I was trying to say is to remove IsCommand parameter and do not rely on return value to determine what we are parsing. We should know what we are doing without that value. If you need to read X and Y, define readX and readY, instead of readZ(bool IsX) or something like that.

Ok, I see. I updated the diff. Updated code looks close to what we had for parsing script file, but it has at least 2 differences atm, so I am not sure it is possible to extract common part (and if we want it here).

ruiu added a comment.Aug 31 2016, 9:58 AM

Obviously, this patch contains duplicate code which does not look good. Let me create a patch based on this to describe what I was trying to say.

In D23609#530513, @ruiu wrote:

Obviously, this patch contains duplicate code which does not look good. Let me create a patch based on this to describe what I was trying to say.

Actually I think I completely understand what you want to see. Something like below probably.
There are still duplications of messages, and only a part of code can be extracted here with no pain.
I can continue working on that tomorrow if you think it is the right direction.

void ScriptParser::readVersionScript() {
  StringRef Msg = "anonymous version definition is used in "
                  "combination with other version definitions";
  if (skip("{")) {
    readVersionDeclaration("");
    if (!atEOF())
      setError(Msg);
    return;
  }

  while (!atEOF() && !Error)
    readNamedVersion();
}

void ScriptParser::readNamedVersion() {
  StringRef Msg = "anonymous version definition is used in "
                  "combination with other version definitions";
  StringRef VerStr = next();
  if (VerStr == "{") {
    setError(Msg);
    return;
  }
  expect("{");
  readVersionDeclaration(VerStr);
}

void ScriptParser::readVersion() {
  expect("{");
  if (skip("{")) {
    readVersionDeclaration("");
    expect("}");
    return;
  }

  while (!Error) {
    readNamedVersion();
    if (peek() == "}")
      break;
  }
  expect("}");
}
grimar abandoned this revision.Aug 31 2016, 1:22 PM

Closing this as r280284 was committed instead.