Page MenuHomePhabricator

Let unaliased Args track which Alias they were created from, and use that in Arg::getAsString() for diagnostics
ClosedPublic

Authored by thakis on Jul 5 2019, 9:03 AM.

Details

Summary

With this, clang-cl /source-charset:utf-16 test.cc now prints invalid value 'utf-16' in '/source-charset:utf-16' instead of invalid value 'utf-16' in '-finput-charset=utf-16' before, and several other clang-cl flags produce much less confusing output as well.

Fixes PR29106.

Since an arg and its alias can have different arg types (joined vs not) and different values (because of AliasArgs<>), I chose to give the Alias its own Arg object. For convenience, I just store the alias directly in the unaliased arg – there aren't many arg objects at runtime, so that seems ok.

Finally, I changed Arg::getAsString() to use the alias's representation if it's present – that function was already documented as being the suitable function for diagnostics, and most callers already used it for diagnostics.

Implementation-wise, Arg::accept() previously used to parse things as the unaliased option. The core of that switch is now extracted into a new function acceptInternal() which parses as the _aliased_ option, and the previously-intermingled unaliasing is now done as an explicit step afterwards.

(This also changes one place in lld that didn't use getAsString() for diagnostics, so that that one place now also prints the flag as the user wrote it, not as it looks after it went through unaliasing.)

Diff Detail

Repository
rL LLVM

Event Timeline

thakis created this revision.Jul 5 2019, 9:03 AM
Herald added a project: Restricted Project. · View Herald Transcript
rnk accepted this revision.Jul 8 2019, 1:04 PM

lgtm

llvm/include/llvm/Option/Arg.h
59 ↗(On Diff #208192)

I'm not sure this is really true:

For convenience, I just store the alias directly in the unaliased arg – there aren't many arg objects at runtime, so that seems ok.

Command lines can end up being quite long. Consider:

$ wc -lc out/clang/browser_tests.exe.rsp
  5381 361257 out/clang/browser_tests.exe.rsp

However, if someone comes along and cares about this, they can try shrinking this SmallVector instead, and using something similar to TinyPtrVector.

This revision is now accepted and ready to land.Jul 8 2019, 1:04 PM

lgtm

Thanks!

5381 361257 out/clang/browser_tests.exe.rsp

Looking at that file, that looks like 10554 distinct args from the rsp file. That's ~80kB for the additional pointer, which for a link that's going to consume hundreds of MB isn't all that much overhead.

(If we did care about Arg size for some reason, there are a whole bunch of things we can do – e.g. removing CommaJoined and the OwnsValues member, or making the Option pointer in the arg and the unaliased arg not duplicate the OptTable pointer, etc.)

This revision was automatically updated to reflect the committed changes.
hans added a comment.Jul 17 2019, 3:07 AM

I think this is going to be my favourite new feature in clang 9 :-) Do you want to put a small note in docs/ReleaseNotes.rst?