Page MenuHomePhabricator

lld: Propeller framework part I
Needs ReviewPublic

Authored by shenhan on Jan 27 2020, 12:17 PM.

Details

Summary

Hi reviewers, this is a split version of the CL - https://reviews.llvm.org/D68062 which seems too big to review and submit in one shot.

What does this CL include:

  1. added Propeller config class PropellerConfig.h
  2. added interface to lld driver in LinkerPropeller.c/cpp.
  3. propeller flags handling

This CL is self-contained, buildable and non-functional. all real work is done in the origin https://reviews.llvm.org/D68062

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

shenhan created this revision.Jan 27 2020, 12:17 PM
Herald added a project: Restricted Project. · View Herald Transcript
shenhan retitled this revision from Propeller to lld: Propeller framework part I.Jan 27 2020, 12:20 PM
shenhan edited the summary of this revision. (Show Details)
shenhan edited reviewers, added: ruiu; removed: espindola.
shenhan added subscribers: tmsriram, rlavaee, rahmanl and 2 others.
shenhan updated this revision to Diff 241588.Jan 30 2020, 2:49 PM
MaskRay edited subscribers, added: grimar; removed: maskray0, MaskRay.

The variable naming does not follow lld's convention (lowerCase).

lld/ELF/Propeller/PropellerConfig.h
12 ↗(On Diff #241588)

lld/Common/LLVM.h pulls in declarations of common utilities. No need for using llvm::StringRef.

lld/include/lld/Common/PropellerCommon.h
17 ↗(On Diff #241588)

Prefer []

28 ↗(On Diff #241588)

excess empty line

46 ↗(On Diff #241588)

Prefer lowerCased variable names.

82 ↗(On Diff #241588)

Rationale? Do you have a concrete example this rule is needed?

95 ↗(On Diff #241588)

Drop unnecessary this

152 ↗(On Diff #241588)
if (s1 && s2)
  return s1->ordinal < s2->ordinal;
return !!s1 < !!s2;
shenhan updated this revision to Diff 241848.Jan 31 2020, 5:08 PM
shenhan marked 5 inline comments as done.

Hi Markray, thanks for the review.

shenhan marked 2 inline comments as done.Jan 31 2020, 5:10 PM
shenhan added inline comments.
lld/include/lld/Common/PropellerCommon.h
46 ↗(On Diff #241588)

I'll address this later, cause this changes almost every other file.

82 ↗(On Diff #241588)

Yeah, we see zero-sized basic block symbol at the end of a function.

shenhan updated this revision to Diff 242370.Feb 4 2020, 10:29 AM

Fixed all variable names to comply with lld naming convention.

shenhan updated this revision to Diff 242462.Feb 4 2020, 4:21 PM
shenhan updated this revision to Diff 243019.Feb 6 2020, 2:55 PM
shenhan edited the summary of this revision. (Show Details)
ruiu added a comment.Feb 7 2020, 4:24 AM

As we discussed today, this patch didn't make sense to me at first sight, as it redoes a lot of things that we already implemented to lld (e.g. making a new "config" object only for this thing, defining a new type of Symbol (SymbolEntry) even though we have Symbol class, etc.) But the thing is that this code is intended to be used by some external tool. If that's the case, you should consider moving this to LLVM instead and use the code from lld and your tool.

shenhan updated this revision to Diff 243328.Feb 7 2020, 5:18 PM

Thanks Rui. Uploaded the patch: 1) removed PropellerConfig 2). Removed PropellerCommon.h from patch (and moved it into llvm realm).

Hi, would you to take another look? Thanks, -Han