Page MenuHomePhabricator

[llvm] llvm-tapi-diff
ClosedPublic

Authored by spowell on May 4 2021, 7:32 AM.

Details

Summary

This patch introduces a new tool, llvm-tapi-diff, that compares and returns the diff of two TBD files.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

fixed patch upload for -U999999

spowell marked 17 inline comments as done.May 10 2021, 12:16 PM
spowell updated this revision to Diff 344217.May 10 2021, 3:25 PM
spowell marked an inline comment as done.

clang-format fixes and replaced macho.dylib for macho.yaml

JDevlieghere added inline comments.May 10 2021, 3:31 PM
llvm/lib/TextAPI/Platform.cpp
134

Should this be llvm::MachO::PlatformKind?

llvm/tools/llvm-tapi-diff/DiffEngine.cpp
2

This seems to be missing a few dashes.

221–222

This is still using 1 and 2 while everything else is now using LHS and RHS.

325–327

The llvm style guidelines say no braces around single-line ifs.

llvm/tools/llvm-tapi-diff/DiffEngine.h
49–59

Rather than documenting the class as a whole, it's good practice to document the individual members like this. This is much more maintainable when a member gets added or removed, and Doxygen will associate the comment with the variable for you, which means you don't have to repeat the type etc.

55–57

Should these members really be public? If the answer is yes, you'd want to use struct instead of class, which has public visibility by default. It's not so much about avoiding the access specifier on line 53, but more about conveying your intention to other developers.

147

This probably also needs an unreachable. Same for the function below. There might be more in this patch.

163

Several files seem to be missing newlines at the end.

spowell updated this revision to Diff 344572.May 11 2021, 3:30 PM

llvm.MachO.PlatformKind => llvm::MachO::PlatformKind in platform.cpp; removed 1 and 2 in favor of LHS and RHS in DiffEngine.cpp; refactored comments in DiffEngine.h; changed DiffOutput from class to struct; added llvm_unreachable in switch statements in DiffEngine.h; Added new lines at end of files.

spowell updated this revision to Diff 344577.May 11 2021, 3:36 PM

added new lines to test input files.

spowell updated this revision to Diff 344579.May 11 2021, 3:40 PM

rebased patch from main to fix patch apply issue, added new line for CMakeLists.txt

spowell updated this revision to Diff 344595.May 11 2021, 4:46 PM

uploading patch again to trigger rebuild

spowell marked 8 inline comments as done.May 12 2021, 7:33 AM
spowell updated this revision to Diff 344841.May 12 2021, 8:46 AM

update separators in tests

spowell updated this revision to Diff 344966.May 12 2021, 2:50 PM

use regex to try to fix windows failure.

spowell updated this revision to Diff 345256.May 13 2021, 12:39 PM

fixed brackets for regex in test

JDevlieghere added inline comments.May 19 2021, 9:46 AM
llvm/tools/llvm-tapi-diff/DiffEngine.cpp
10–11

This is not a header tho :-)

93–96

Could we reduce the density of this code by adding a helper that looks something like:

void DiffAttribute(StringRef Name, std::vector<DiffOutput>& Output, AttributeDiff Attr) {
  Output.push_back(getSingleAttrDiff(Attr, Name));
}

and then simplify these calls to:

DiffAttribute("Install Name", ReturnedOutput, DiffScalarVal<StringRef, AD_Diff_Scalar_Str>(Order, Interface->getInstallName()));

Having the name of the attribute first makes it clear what you're diffing. I'd probably also rename ReturnedOutput to something shorter (Output) to reduce the length of those calls.

llvm/tools/llvm-tapi-diff/DiffEngine.h
65–66

Can these two be private? These members look like implementation details. Same in the other classes.

73

I see (this->Order == lhs) ? "< " : "> ") in several places. How about creating a helper that takes a InterfaceInputOrder and returns a StringRef to avoid this duplication?

133

Why are these functions implemented in the header?

172

The LLVM Coding Guide says that comments should be sentences, i.e. capitalized and with a period at the end.

223

You're passing the vector by value, so this will copy all its element for printing. Can this be a const ref?

cishida added inline comments.May 19 2021, 7:32 PM
llvm/tools/llvm-tapi-diff/DiffEngine.cpp
129

you don't need this conditional, the body of the for loop would never run if its empty

172

These don't need to be scoped

llvm/tools/llvm-tapi-diff/DiffEngine.h
57

Most, if not all, instances of this type are named Diff Diff.DiffVec is redundant and is also used as a variable name in other places.
What about Values?

82

nit: this-> calls are unecessarcy

102

Remove the typo at the end.

llvm/tools/llvm-tapi-diff/llvm-tapi-diff.cpp
2

sorry for the confusion on notation, I mean either tapi or tbd should be in the description.

Hey, just a few notes on proper error handling.

llvm/tools/llvm-tapi-diff/llvm-tapi-diff.cpp
41

The idiomatic solution for this is ExitOnErr as described here:
https://llvm.org/docs/ProgrammersManual.html#using-exitonerror-to-simplify-tool-code

53

Could the result be returned directly here? Like this?

return createBinary(BufferOrErr.get()->getMemBufferRef());
74

I think with ExitOnError this could be:

std::unique_ptr<Binary> BinLHS = ExitOnErr(convertFileToBinary(InputFileNameLHS));

It is not uncommon to have it as a static global in a tool's driver cpp and use it directly all over the place. There is a number of good examples, e.g. llvm-link https://github.com/llvm/llvm-project/blob/81bc732816107f6aff4fd61980f7b03cc92332b5/llvm/tools/llvm-link/llvm-link.cpp#L296

spowell updated this revision to Diff 347398.May 24 2021, 8:05 AM

fixed heading on DiffEngine.cpp; made helper to collect single scalar values; ReturnedOutput => Output; made implementation details private for class variables; made helper to set order indicator for print functions; moved functions that did not need to be in header; removed unneeded conditional; removed unneeded scope; DiffVec => Values, TargValues, DocValues; addValToDiffVec => addDiffToTargetSlice; removed unnecessary this-> calls; removed typo in comment; fixed heading on llvm-tapi-diff.cpp; fixed error handling in llvm-tapi-diff.cpp

spowell marked 16 inline comments as done.May 24 2021, 8:06 AM
spowell updated this revision to Diff 347424.May 24 2021, 9:27 AM

removed import that was auto-added by text editor causing failure of tests.

spowell updated this revision to Diff 347433.May 24 2021, 10:06 AM

fix error handling in llvm-tapi-diff.cpp

A few small nits and this should be ready to go.

llvm/tools/llvm-tapi-diff/DiffEngine.cpp
384–387

LHS? DocLHS?

458

This line got me thinking that it might be nice to make the raw_ostream an argument to printDifferences and do the same for compareFiles below. That aligns nicely with LLVM's library design, where the actual implementation (the DiffEngine) can remain unaware of where its output is being printed while centralize the fact that it's going to stdout in llvm-tapi-diff.cpp.

llvm/tools/llvm-tapi-diff/llvm-tapi-diff.cpp
30–33

This is generally lowercase.

spowell updated this revision to Diff 349047.Jun 1 2021, 12:09 PM

Doc1 => DocLHS; Doc2 => DocRHS; made raw_ostream argument in compareFiles and printDifferences; lowercase argument prompts;

spowell marked 3 inline comments as done.Jun 1 2021, 12:10 PM
JDevlieghere accepted this revision.Jun 1 2021, 12:19 PM

LGTM! Thanks for bearing with me :-)

This revision is now accepted and ready to land.Jun 1 2021, 12:19 PM
spowell updated this revision to Diff 349102.Jun 1 2021, 2:36 PM

formatting fixes

ributzka accepted this revision.Jun 2 2021, 9:35 AM

LGTM

This revision was landed with ongoing or failed builds.Jun 3 2021, 11:40 AM
Closed by commit rGd1d36f7ad2ae: [llvm] llvm-tapi-diff (authored by spowell, committed by cishida). · Explain Why
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Jun 3 2021, 4:21 PM
thakis added inline comments.
llvm/test/tools/llvm-tapi-diff/tapi-diff-matching-tbd.test
1

Shouldn't you add llvm-tapi-diff to llvm/test/CMakeLists.txt to make sure it gets built when you build check-llvm? And to the tools.extend( bit in llvm/test/lit.cfg.py?

jyknight added inline comments.
llvm/tools/llvm-tapi-diff/DiffEngine.h
57

std::vector<std::unique_ptr<AttributeDiff>>

Storing a subclass of AttributeDiff into a unique_ptr<AttributeDiff> is not OK, because AttributeDiff does not have a virtual destructor. So, this tool is invoking UB.

Just adding the virtual destructor to AttributeDiff is probably ok -- there doesn't seem to be any reason to avoid using virtual dispatch in this tool's class hierarchy, afaict.

The tests for this tool are observed to be failing on multiple big-endian platforms:
clang-s390x-linux: https://lab.llvm.org/buildbot/#/builders/94/builds/4116
clang-ppc64be-linux: https://lab.llvm.org/buildbot/#/builders/52/builds/8035

The tests for this tool are observed to be failing on multiple big-endian platforms:
clang-s390x-linux: https://lab.llvm.org/buildbot/#/builders/94/builds/4116
clang-ppc64be-linux: https://lab.llvm.org/buildbot/#/builders/52/builds/8035

Thanks for letting us know! @spowell from reading the lit output, it appears like the output order for the target slices isn't stable. I've reverted while we investigate and fix.

I'm pretty sure this also causes a build failure on older, but supported versions of GCC due to this bug (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56480):

../tools/llvm-tapi-diff/DiffEngine.cpp:35:71: error: specialization of template<class T, llvm::DiffAttrKind U> void llvm::DiffScalarVal<T, U>::print(llvm::raw_ostream&, std::__cxx11::string) in different namespace [-fpermissive]
                                                     std::string Indent) {
                                                                       ^