This is an archive of the discontinued LLVM Phabricator instance.

Handle ":" as a regular token character in linker scripts.
ClosedPublic

Authored by ruiu on Mar 7 2017, 3:21 PM.

Event Timeline

ruiu created this revision.Mar 7 2017, 3:21 PM
ruiu updated this revision to Diff 90958.Mar 7 2017, 3:53 PM
  • Remove the parameter from peek().
ruiu updated this revision to Diff 90965.Mar 7 2017, 4:50 PM
  • Remove a use of Twine. We don't need that.
grimar edited edge metadata.Mar 9 2017, 1:30 AM

This approach sure has simpler implementation,
(when I wrote my patch, I sure thought about doing the same),
but it may open Pandora's box.

For example it is easy to copypaste "global" instead of "local" and it be accepted:

{ 
global: 
  aaa;
global: *; 
};

At fact any combinations of local and global labels are possible with it:

{
 global:
  aa;
 local:
  bb;
 global:
  cc;
 local:
  dd;
}

That opens road to writing overcomplicated and what is probably
more important - a road to write scripts that are incompatible with both BFD and gold !
I think compatibility and simplicity is what we want to deliver and if gnu linkers do not
accept that, I see that is a reason also not to accept it.
At fact you want to accept such syntax because its simplifies implementation and
that is understandable position, though I think we probably should not do that here.

ruiu added a comment.Mar 9 2017, 9:56 AM

This approach sure has simpler implementation,
(when I wrote my patch, I sure thought about doing the same),
but it may open Pandora's box.

For example it is easy to copypaste "global" instead of "local" and it be accepted:

{ 
global: 
  aaa;
global: *; 
};

At fact any combinations of local and global labels are possible with it:

{
 global:
  aa;
 local:
  bb;
 global:
  cc;
 local:
  dd;
}

That opens road to writing overcomplicated and what is probably
more important - a road to write scripts that are incompatible with both BFD and gold !
I think compatibility and simplicity is what we want to deliver and if gnu linkers do not
accept that, I see that is a reason also not to accept it.
At fact you want to accept such syntax because its simplifies implementation and
that is understandable position, though I think we probably should not do that here.

I disagree. If you intentionally try to break a thing, it will break, but that's not realistic. Your examples are clearly wrong and easy to fix for programmers. If you know the internal implementation of LLD and other linkers, you can always write a linker script that is broken in a subtle way in some linker but works perfectly fine with different one. For example, our lexer behaves different in corner cases from the GNU linker's because we aimed simplicity, and if you know about that, you can come up with many examples that doesn't work with our linker or their linkers. I think you are overthinking.

ruiu added a comment.Mar 9 2017, 10:00 AM

Note that we can easily reject linker scripts that you gave me as bad examples just by counting the number of occurrences of each label in the function and doesn't allow more than one label for each label name. That's worth to do though.

ruiu added a comment.Mar 9 2017, 10:03 AM

That's worth to do though.

That's _not_

grimar accepted this revision.Mar 9 2017, 10:08 AM

Implementation is LGTM. My concern stands, but we can change it to stricter behavior separatelly.

This revision is now accepted and ready to land.Mar 9 2017, 10:08 AM
ruiu added a comment.Mar 9 2017, 10:10 AM

Yes, if that would raise a real issue, I can add counters to the function.

This revision was automatically updated to reflect the committed changes.