Page MenuHomePhabricator

[Try 2] Include sanitize blacklist and other extra deps as part of scan-deps output
AbandonedPublic

Authored by kousikk on Oct 17 2019, 12:29 AM.

Details

Summary

Clang's -M mode includes these extra dependencies in its output and clang-scan-deps
should have equivalent behavior, so adding these extradeps to output just like
how its being done for ".d" file generation mode.

This is a retry of https://reviews.llvm.org/rC375074. From the build-bot failure on
Windows, it seemed like somehow the blacklist file was already added as a dependency.
So the extra change in this patch is that I add deps to a set in clang-scan-deps
to eliminate duplicates and print in sorted order. Having a set achieves two purposes:

  1. Prints out the dependencies in sorted order
  2. Eliminates duplicates

Windows bot failure: http://lab.llvm.org:8011/builders/clang-x64-ninja-win7/builds/15956/steps/ninja%20check%202/logs/stdio

An alternative fix to the test would have been to check only for the presence of
blacklsit file, but I didn't prefer it since explicit testing of all expected outputs
is better (and in this case has led us to capture duplicate outputs).

Event Timeline

kousikk created this revision.Oct 17 2019, 12:29 AM
kousikk edited the summary of this revision. (Show Details)Oct 17 2019, 12:30 AM
kousikk updated this revision to Diff 225375.Oct 17 2019, 1:40 AM

Fix tests now that output is in ascending order

I think you could've just used CHECK-DAG to fix the tests. It *might* be a bit more robust. Although just reordering checks seems perfectly fine too.
https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-dag-directive

Using std::set here sounds reasonable to me but I'd like @arphaman to be aware of this. BTW should we change std::vector<std::string> Dependencies; in DependencyCollector to set too?

Also, I'm wondering why is the behavior different on Windows.

Thanks for the comment @jkorous.

I think you could've just used CHECK-DAG to fix the tests. It *might* be a bit more robust. Although just reordering checks seems perfectly fine too. https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-dag-directive. Using std::set here sounds reasonable to me but I'd like @arphaman to be aware of this.

Thanks for pointing me to it - I have a minor preference towards a sorted order of outputs vs order in which we visit the files. I think a sorted order is much more easy to reason about for a client. Having said that, @arphaman what do you think? If you and Jan both feel that we should maintain the current order, I'll stick to that and not change Dependencies to set.

BTW should we change std::vector<std::string> Dependencies; in DependencyCollector to set too?

I think this would change the output of *.d file? If so I'm a little hesitant to make that change because of the impact it would have, but once again I'll defer to your judgement on this (I'm very new to LLVM codebase).

Also, I'm wondering why is the behavior different on Windows.

I confirmed that there are duplicate blacklist.txt files without changing vector to set. I'm setting up a Windows machine to try to figure out why that is the case. Will share what I find.

Thanks for the comment @jkorous.

I think you could've just used CHECK-DAG to fix the tests. It *might* be a bit more robust. Although just reordering checks seems perfectly fine too. https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-dag-directive. Using std::set here sounds reasonable to me but I'd like @arphaman to be aware of this.

Thanks for pointing me to it - I have a minor preference towards a sorted order of outputs vs order in which we visit the files. I think a sorted order is much more easy to reason about for a client. Having said that, @arphaman what do you think? If you and Jan both feel that we should maintain the current order, I'll stick to that and not change Dependencies to set.

Sorting seems reasonable to me, perhaps behind an option (I'll leave that up to others). But you don't need a std::set for this, just run llvm::sort and std::unique once the dependencies are collected.

I was able to test this on a Windows machine. The ExtraDeps are already being added to the dependency output (this code does that - https://github.com/llvm/llvm-project/blob/master/clang/lib/Frontend/DependencyFile.cpp#L188). My suspicion for why I wasn't seeing them previously was that these ExtraDeps are added as relative paths and my consumer code (which integrates with scan-deps) was ignoring relative paths. I can handle that in the consumer code - I'll leave the behaviour of the tool as it is.

Sorry about the back and forth on this. I'm closing this out.

kousikk abandoned this revision.Oct 21 2019, 1:02 PM