Page MenuHomePhabricator

[LLD][COFF] Support /ignore:4099. Support /ignore with comma-separated arguments.
ClosedPublic

Authored by aganea on Jan 7 2019, 7:56 AM.

Details

Summary

Ditto.

/ignore:4099 corresponds to "PDB 'filename' was not found with 'object/library' or at 'path'; linking object as if no debug info".

Without /ignore:4099, we can't enable /WX (treat warnings as errors).

Also, providing multiple arguments such as /ignore:4037,4049,4065,4098,4099,4217,4221 now works.

Diff Detail

Repository
rL LLVM

Event Timeline

aganea created this revision.Jan 7 2019, 7:56 AM
This revision is now accepted and ready to land.Jan 7 2019, 10:29 AM
ruiu added a subscriber: ruiu.Jan 7 2019, 10:39 AM
ruiu added inline comments.
COFF/Driver.cpp
991 ↗(On Diff #180499)

What StringRef::split returns is not an iterator, so I think It is not a very good name. Since this is not a performance-critical path, maybe this is the simplest code I could think of:

SmallVector<StringRef, 1> Vec;
StringRef(Arg->getValue())->split(Vec, ',');
for (StringRef S : Vec) {
  if (S == "4037") {
    ...

You can find the same pattern in the code handling OPT_opt.

COFF/PDB.cpp
1304 ↗(On Diff #180499)

Flip the condition to return early.

1305 ↗(On Diff #180499)

Please avoid using auto unless its type is obvious from the right-hand side (for example, if the rhs is a dyn_cast, then auto is fine, but in this case the actual type is not obvious).

1306 ↗(On Diff #180499)

I don't know if adding [LNK4099] is the right thing. We have a lot of warnings and error messages in lld, but no one includes such error code. If we want to do this (I personally don't want though), we should do for everything, but this needs to be discussed first.

aganea marked an inline comment as done.Jan 7 2019, 10:47 AM
aganea added inline comments.
COFF/PDB.cpp
1306 ↗(On Diff #180499)

This is how other /ignore are handled (look for "[LNK4037]" and "[LNK4217]" in lld/trunk/COFF/Driver.cpp and lld/trunk/COFF/SymbolTable.cpp). This has been discussed in D44297.

aganea updated this revision to Diff 180543.Jan 7 2019, 12:01 PM
aganea marked 3 inline comments as done.

Updated as requested by @ruiu
Please let me know if that sounds better.

aganea updated this revision to Diff 180544.Jan 7 2019, 12:07 PM

Sorry I missed one line in PDB.cpp. Should be good now.

ruiu accepted this revision.Jan 7 2019, 1:20 PM

LGTM

test/COFF/ignore-many.yaml
1 ↗(On Diff #180544)

This file is not a YAML file, so let's not use the .yaml file extension. Please name this ignore-many.test.

17 ↗(On Diff #180544)

Remove the extraneous last '\n'. (Your file ends with "\n\n", so you should remove the last '\n").

This revision was automatically updated to reflect the committed changes.