[PATCH] D46610: [CMake] Set CMAKE_BUILD_WITH_INSTALL_RPATH for Fuchsia runtimes

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

Need some clarification on this option

1. The rpath is the path from which libs mentioned in .dynamic are relative to?
2. Normally cmake first links binaries without setting the RPATH and then sets 
it later.
3. This makes RPATH always equal to the empty string and avoids the "relinking" 
step that resets RPATH

If so this LGTM


Repository:
  rC Clang

https://reviews.llvm.org/D46610



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46610: [CMake] Set CMAKE_BUILD_WITH_INSTALL_RPATH for Fuchsia runtimes

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich accepted this revision.
jakehehrlich added a comment.
This revision is now accepted and ready to land.

Spoke offline, LGTM


Repository:
  rC Clang

https://reviews.llvm.org/D46610



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46791: Make -gsplit-dwarf generally available

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added inline comments.



Comment at: lib/Driver/ToolChains/Clang.cpp:4818
   // Handle the debug info splitting at object creation time if we're
   // creating an object.
   if (SplitDWARF && Output.getType() == types::TY_Object)

Maybe re-add a TODO to point out that *some* objcopy is needed be it GNU 
objcopy or llvm-objcopy? Ideally this would be done by the compiler and not by 
an external tool.



Comment at: test/Driver/split-debug.c:7
 //
 // CHECK-ACTIONS: objcopy{{.*}}--extract-dwo{{.*}}"split-debug.dwo"
 // CHECK-ACTIONS: objcopy{{.*}}--strip-dwo{{.*}}"split-debug.o"

I think I may have missed something here. I thought this would change all of 
clang's objcopy invocations regardless of the target triple. What chooses 
"objcopy" if the target system is linux or linux-gnu? I think this might be 
passing because that line still matches "llvm-objcopy ..."



Comment at: test/Driver/split-debug.c:14
+//
+// FREEBSD-NOIA-CHECK-ACTIONS: 
llvm-objcopy{{.*}}--extract-dwo{{.*}}"split-debug.dwo"
+// FREEBSD-NOIA-CHECK-ACTIONS: 
llvm-objcopy{{.*}}--strip-dwo{{.*}}"split-debug.o"

When I wrote this I wanted this test to pass regardless of which objcopy was 
used so I just left it as "objcopy" because that would still pass if 
"llvm-objcopy" or "objcopy" was used. For instance my team builds the toolchain 
and we consider a build a fail of all tests don't pass. If someone wanted to 
use a GNU objcopy and built and tested toolchains the same way they would run 
into this issue.


Repository:
  rC Clang

https://reviews.llvm.org/D46791



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46791: Make -gsplit-dwarf generally available

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

I'm generally ok with this but I think some important checks need to occur 
first. Namely I think I'd like to check that Chromium still builds with this 
option since I'd imagine they'd be affected by this.


Repository:
  rC Clang

https://reviews.llvm.org/D46791



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46791: Make -gsplit-dwarf generally available

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

Yeah I haven't heard back from Lexan nor have I tried reproducing such a build 
myself and if we're that close to not relaying on this sort of hack I'd much 
rather wait on this than risk breaking someone for the sake of a stopgap for 
such a short period of time. @echristo Can you link the review to that?


https://reviews.llvm.org/D46791



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

LGTM with a couple additions.




Comment at: lib/CodeGen/CodeGenTypes.cpp:448
+case BuiltinType::ULongAccum:
   ResultType = llvm::IntegerType::get(getLLVMContext(),
  
static_cast(Context.getTypeSize(T)));

Add TODO for accum types using a different drawf type.



Comment at: lib/Index/USRGeneration.cpp:731
+
+  if (c == '~') {
+switch (BT->getKind()) {

You can make the 'c' a Twine instead. That will let you inline these in their 
respective locations as `  c = "~UA" ` for instance.


Repository:
  rC Clang

https://reviews.llvm.org/D46084



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47229: Make atomic non-member functions as nonnull

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

This is causing breaks in fuchsia,

Code that looks like this

  uintptr_t last_unlogged =
   atomic_load_explicit(&unlogged_tail, memory_order_acquire);
   do { 
   if (last_unlogged == 0)
   return;
   } while (!atomic_compare_exchange_weak_explicit(&unlogged_tail,
   &last_unlogged, 0,
   memory_order_acq_rel,
   memory_order_relaxed));

Where unlogged_tail is somewhere on the stack. And 
'atomic_compare_exchange_weak_explicit' is an #define alias for 
'__c11_atomic_compare_exchange_weak'

Full context here: 
https://fuchsia.googlesource.com/zircon/+/master/third_party/ulib/musl/ldso/dynlink.c#822


Repository:
  rL LLVM

https://reviews.llvm.org/D47229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49628: [CMake] Link static libunwind and libc++abi into libc++ in Fuchsia toolchain

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich accepted this revision.
jakehehrlich added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rC Clang

https://reviews.llvm.org/D49628



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50208: [clang-doc] Fix unique_ptr error on bots

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich accepted this revision.
jakehehrlich added a comment.
This revision is now accepted and ready to land.

Make sure the commit message reflects this change but other than that LGTM


https://reviews.llvm.org/D50208



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41102: Setup clang-doc frontend framework

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

Additional note: This diff is a diff from your last commit not the full diff 
relative to origin/master which is what should be up here.




Comment at: tools/clang-doc/ClangDoc.cpp:34
 
-bool ClangDocVisitor::VisitNamespaceDecl(const NamespaceDecl *D) {
+template <>
+void ClangDocCallback::processMatchedDecl(

I can't think of a good way to dedup these two methods at the moment. Can you 
put a TODO here to deduplicate these two specializations?



Comment at: tools/clang-doc/ClangDoc.h:69
 
-private:
-  ClangDocVisitor Visitor;
-  ClangDocReporter &Reporter;
-};
-
-class ClangDocAction : public clang::ASTFrontendAction {
-public:
-  ClangDocAction(ClangDocReporter &Reporter) : Reporter(Reporter) {}
-
-  virtual std::unique_ptr
-  CreateASTConsumer(clang::CompilerInstance &C, llvm::StringRef InFile);
+  virtual void HandleTranslationUnit(clang::ASTContext &Context) {
+Finder->matchAST(Context);

This should be moved to the .cpp file. Because there is no key function 
(https://itanium-cxx-abi.github.io/cxx-abi/abi.html#vague-vtable) this method 
will be redefined in every translation unit that includes this header.



Comment at: tools/clang-doc/ClangDocReporter.cpp:422
+void ClangDocReporter::serializeLLVM(StringRef RootDir) {
+  // TODO: Implement.
+}

jakehehrlich wrote:
> Can you report an error to the user that says something along the lines of 
> "not implemented yet" (leave the TODO as well)
I think it would be better if instead of returning a string, you just fail and 
print a message to the user (well, first print the message and then fail).



Comment at: tools/clang-doc/ClangDocReporter.cpp:41
+  Docs.Files[Filename] = llvm::make_unique();
+  Docs.Files[Filename]->Filename = Filename;
 }

nit: Can this just do one lookup?

```
auto F = llvm::make_unique();
F->Filename = Filename;
Docs.Files[Filename] = std::move(Filename);
```



Comment at: tools/clang-doc/ClangDocReporter.cpp:136
+  if (NS == Docs.Namespaces.end()) {
+Docs.Namespaces[Namespace] = llvm::make_unique();
+Docs.Namespaces[Namespace]->FullyQualifiedName = Namespace;

nit: could you rewrite with a single lookup.



Comment at: tools/clang-doc/ClangDocReporter.cpp:190
 return;
-  CommentInfo CI;
-  parseFullComment(C, CI);
-  I.Descriptions.push_back(CI);
+  I.Descriptions.push_back(std::move(parseFullComment(C)));
 }

If you use emplace_back here you don't need the explicit std::move



Comment at: tools/clang-doc/ClangDocReporter.cpp:281-288
+  sys::path::native(RootDir, FilePath);
+  sys::path::append(FilePath, "files.yaml");
+  raw_fd_ostream FileOS(FilePath, OutErrorInfo, sys::fs::F_None);
+  if (OutErrorInfo != OK) {
+errs() << OutErrorInfo.message();
+errs() << " Error opening documentation file.\n";
+return;

You use the same basic code 3 times for different file names. Can you factor 
that out into a function? Also in this block you output the OutErrorInfo 
message but in blocks below you don't. You should always output that message.



Comment at: tools/clang-doc/ClangDocReporter.cpp:335
+  RootCI->Kind = C->getCommentKindName();
+  CurrentCI = RootCI.get();
+  ConstCommentVisitor::visit(C);

Instead of assigning a CI like this, could you construct a new 
ClangDocCommentVisitor on the stack? The idea would be that you could would 
still have a "CI" member variable that would be set in the 
ClangDocCommentVisitor's constructor. That way it never has to change and each 
visitor is just responsible for constructing one CommentInfo





Comment at: tools/clang-doc/ClangDocReporter.h:168
 private:
+  template 
+  void createInfo(llvm::StringMap> &InfoMap, const C *D,

If you add a public "using DeclType = FooDecl;" to each "FooInfo" you can 
eliminate the second template argument and make the intent of this code more 
clear. This also formalizes the connection these types have to each other.



Comment at: tools/clang-doc/tool/ClangDocMain.cpp:76
+  if (DirectoryStatus != OK) {
+errs() << "Unable to create documentation directories.\n";
+return 1;

Can you convert this error_code to a message and display that to the user?


https://reviews.llvm.org/D41102



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41102: Setup clang-doc frontend framework

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

After the comments I just made are resolved, I'm fine with the low level 
details of the parts of this change that are here




Comment at: clang-doc/ClangDoc.cpp:47
+
+comments::FullComment *ClangDocCallback::getComment(const NamedDecl *D) {
+  RawComment *Comment = Context->getRawCommentForDeclNoCache(D);

I don't see any reason this can't be a const method. If I recall a previous 
version you said that it can be it can't be const because it modifies the 
Comment but that shouldn't violate the this being a const method. No 
modifications are being made to any members of this object and no non-const 
references/pointers to any of the members are accessed or needed.



Comment at: clang-doc/ClangDocMapper.cpp:91
+  ClangDocCommentVisitor Visitor(CI);
+  return Visitor.parseComment(C);
+}

If/When you make CurrentCI a reference you should return CI here instead.



Comment at: clang-doc/ClangDocMapper.cpp:181
+  for (comments::Comment *Child : make_range(C->child_begin(), 
C->child_end())) {
+CommentInfo ChildCI;
+ClangDocCommentVisitor Visitor(ChildCI);

Assuming you make the reference changes above, this should be rewritten to 
something like the following:

CurrentCI.Children.emplace_back();
ClangDocCommentVisitor Visitor(CurrentCI.Children.back());
Visitor.parseComment(Child);



Comment at: clang-doc/ClangDocMapper.cpp:266
+bool ClangDocCommentVisitor::isWhitespaceOnly(StringRef S) const {
+  return S.find_first_not_of(" \t\n\v\f\r") == std::string::npos;
+}

Can you use isspace here instead of keeping a list of characters that are 
considered spaces?



Comment at: clang-doc/ClangDocMapper.h:112
+  
+  CommentInfo parseComment(const comments::Comment *C);
+

This shouldn't return CommentInfo if you make CurrentCI a reference



Comment at: clang-doc/ClangDocMapper.h:129
+  
+  CommentInfo CurrentCI;
+};

This should be a CommentInfo& to avoid copy constructing. It also then lets you 
view ClangDocCommentVisitor as a kind of CommentInfo builder


https://reviews.llvm.org/D41102



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41102: Setup clang-doc frontend framework

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

Can we add tests for a function at the top level and for a method?


https://reviews.llvm.org/D41102



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43392: [clang-tidy] Add Fuchsia checker for visibility attributes

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

I'd like a test for #pragma GCC visibility push(hidden) which should cause each 
symbol to be treated as though it were explicitly hidden.




Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:32
+  if (Name.empty()) return;
+  Finder->addMatcher(functionDecl(allOf(hasName(Name), isDefinition(),
+unless(hasVisibilityAttr(

aaron.ballman wrote:
> Would it make more sense to store a list of names and then find all of them 
> at once, rather than a single name at a time?
+1 on this we generally want to make this change to many different functions at 
the same time.


https://reviews.llvm.org/D43392



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43392: [clang-tidy] Add Fuchsia checker for visibility attributes

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

> I'd like a test for #pragma GCC visibility push(hidden) which should cause 
> each symbol to be treated as though it were explicitly hidden.

To clarify, this check should still give a warning if the specified variable 
doesn't actually have that in source code. e.g. simply because "#pragma GCC 
visibility push(hidden)" was at the top dosn't mean these functions should get 
a pass.


https://reviews.llvm.org/D43392



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43392: [clang-tidy] Add Fuchsia checker for visibility attributes

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added inline comments.



Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:68
+  diag(MatchedDecl->getLocStart(),
+   "visibility attribute not set for specified function")
+  << MatchedDecl

aaron.ballman wrote:
> How about: "function expected to be annotated with '%0' visibility"
> 
> I'm mostly worried about the case where the function has a visibility 
> attribute, but it has the *wrong* visibility specified. The current wording 
> would be confusing in that circumstance. Or is that not a scenario you care 
> about, just that *any* visibility is specified on the function?
The use case for this check is forcing code bases to carefully control what 
symbols are exported rather than just exporting everything. So if someone took 
the time to explicitly set the visibility of one of these symbols we care about 
then we should assume they knew what they were doing.

The specific use case I care about for this check is using -fvisibility=hidden 
and then checking to make sure a certain curated list of symbols has explicit 
default visibility.


https://reviews.llvm.org/D43392



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43404: [Fuchsia] Include libClang and clang-include-fixer in the toolchain

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich accepted this revision.
jakehehrlich added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rC Clang

https://reviews.llvm.org/D43404



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43545: [Driver] Make -fno-common default for Fuchsia

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich accepted this revision.
jakehehrlich added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rC Clang

https://reviews.llvm.org/D43545



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41102: Setup clang-doc frontend framework

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added inline comments.



Comment at: clang-doc/BitcodeWriter.cpp:407
+
+void ClangDocBinaryWriter::writeBitstream(const EnumInfo &I,
+  BitstreamWriter &Stream,

lebedev.ri wrote:
> juliehockett wrote:
> > lebedev.ri wrote:
> > > Hmm, common pattern again
> > > ```
> > > void ClangDocBinaryWriter::writeBitstream(const  &I,
> > >   BitstreamWriter &Stream,
> > >   bool writeBlockInfo) {
> > >   if (writeBlockInfo) emitBlockInfoBlock(Stream);
> > >   StreamSubBlock Block(Stream, BI__BLOCK_ID);
> > >   ...
> > > }
> > > ```
> > > Could be solved if a mapping from `TYPENAME` -> 
> > > `BI__BLOCK_ID` can be added.
> > > If LLVM would be using C++14, that'd be easy, but with C++11, it would 
> > > require whole new class (although with just a single static variable).
> > Do you want me to try to write that class, or leave it as it is?
> It would be something like: (total guesswork, literally just wrote it here, 
> like rest of the snippets)
> ```
> template 
> struct MapFromTypeToEnumerator {
>   static const BlockId id;
> };
> 
> template <>
> struct MapFromTypeToEnumerator {
>   static const BlockId id = BI_NAMESPACE_BLOCK_ID;
> };
> void ClangDocBitcodeWriter::writeBitstream(const NamespaceInfo &I) {
>   EMITINFO(NAMESPACE)
> }
> ...
> 
> template 
> void ClangDocBitcodeWriter::writeBitstream(const TypeInfo &I, bool 
> writeBlockInfo) {
>   if (writeBlockInfo) emitBlockInfoBlock();
>   StreamSubBlockGuard Block(Stream, MapFromTypeToEnumerator::id);
>   writeBitstream(I);
> }
> ```
> Uhm, now that i have wrote it, it does not look as ugly as i though it would 
> look...
> So maybe try integrating that, i *think* it is a bit cleaner?
If we know the set of types then it should just be a static member of every 
*Info type. Then the mapping is just TYPENAME::id


https://reviews.llvm.org/D41102



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41102: Setup clang-doc frontend framework

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added inline comments.



Comment at: clang-doc/BitcodeWriter.cpp:407
+
+void ClangDocBinaryWriter::writeBitstream(const EnumInfo &I,
+  BitstreamWriter &Stream,

juliehockett wrote:
> jakehehrlich wrote:
> > lebedev.ri wrote:
> > > juliehockett wrote:
> > > > lebedev.ri wrote:
> > > > > Hmm, common pattern again
> > > > > ```
> > > > > void ClangDocBinaryWriter::writeBitstream(const  &I,
> > > > >   BitstreamWriter &Stream,
> > > > >   bool writeBlockInfo) {
> > > > >   if (writeBlockInfo) emitBlockInfoBlock(Stream);
> > > > >   StreamSubBlock Block(Stream, BI__BLOCK_ID);
> > > > >   ...
> > > > > }
> > > > > ```
> > > > > Could be solved if a mapping from `TYPENAME` -> 
> > > > > `BI__BLOCK_ID` can be added.
> > > > > If LLVM would be using C++14, that'd be easy, but with C++11, it 
> > > > > would require whole new class (although with just a single static 
> > > > > variable).
> > > > Do you want me to try to write that class, or leave it as it is?
> > > It would be something like: (total guesswork, literally just wrote it 
> > > here, like rest of the snippets)
> > > ```
> > > template 
> > > struct MapFromTypeToEnumerator {
> > >   static const BlockId id;
> > > };
> > > 
> > > template <>
> > > struct MapFromTypeToEnumerator {
> > >   static const BlockId id = BI_NAMESPACE_BLOCK_ID;
> > > };
> > > void ClangDocBitcodeWriter::writeBitstream(const NamespaceInfo &I) {
> > >   EMITINFO(NAMESPACE)
> > > }
> > > ...
> > > 
> > > template 
> > > void ClangDocBitcodeWriter::writeBitstream(const TypeInfo &I, bool 
> > > writeBlockInfo) {
> > >   if (writeBlockInfo) emitBlockInfoBlock();
> > >   StreamSubBlockGuard Block(Stream, 
> > > MapFromTypeToEnumerator::id);
> > >   writeBitstream(I);
> > > }
> > > ```
> > > Uhm, now that i have wrote it, it does not look as ugly as i though it 
> > > would look...
> > > So maybe try integrating that, i *think* it is a bit cleaner?
> > If we know the set of types then it should just be a static member of every 
> > *Info type. Then the mapping is just TYPENAME::id
> @jakehehrlich The issue with that is that it would mix writer implementation 
> details into the representation, which at this point has no knowledge of the 
> writer. We can break that, but is that the best option?
Probably not, I didn't think about that.


https://reviews.llvm.org/D41102



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43392: [clang-tidy] Add Fuchsia checker for visibility attributes

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added inline comments.



Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:26
+
+// AST_MATCHER(FunctionDecl, isInHeaderFile) {
+//   return Node.getExplicitVisibility(NamedDecl::VisibilityForType);

What are these comments doing here?



Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:49
+Vis = DefaultVisibility;
+  else
+   llvm::errs() << "Invalid visibliity attribute: " << VisAttr << "\n";

What about internal?



Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:64
+  Names.end())),
+ unless(hasVisibilityAttr(Vis
+  .bind("no-visibility"),

Something in the list that simply has explicit visibility should pass I think. 
For instance saying you have a blacklist of symbols instead of a whitelist. 
Sometimes internal or hidden might be used but we might want to add "hidden" by 
default. So "Vis" might be DefaultVisibility but we don't want to raise an 
error/change something labeled with internal visibility to hidden.


https://reviews.llvm.org/D43392



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43392: [clang-tidy] Add Fuchsia checker for visibility attributes

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

Can you add a test for when #pragma GCC visibility push(hidden) is used and the 
visibility attribute is hidden?


https://reviews.llvm.org/D43392



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53081: [clang-doc] Add unit tests for serialization

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich accepted this revision.
jakehehrlich added a comment.
This revision is now accepted and ready to land.

LGTM, I'd file a bug about SmallString not taking const char* in its 
constructor and I'd also put a TODO somewhere to fix that once that's resolved.


https://reviews.llvm.org/D53081



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53083: [clang-doc] Add unit tests for merging

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich accepted this revision.
jakehehrlich added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/unittests/clang-doc/MergeTest.cpp:222
+
+  Expected->DefLoc = Location(10, llvm::SmallString<16>("test.cpp"));
+  Expected->Loc.emplace_back(12, llvm::SmallString<16>("test.cpp"));

Use {} not () on both SmallString and Location (and any other constructors)


https://reviews.llvm.org/D53083



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53084: [clang-doc] Add unit tests for YAML

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

This just seems like a lit test in unit test form, why does this need to use 
unit tests and not lit tests?


https://reviews.llvm.org/D53084



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

Can you elaborate on the use case for this? Like can you explain end to end how 
this would be used?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

In D60974#1474751 , @plotfi wrote:

> There are a few that I have in mind.
>
> 1. -emit-ifso could be added to a build to produce .so files as part of a 
> device SDK (where we don't want to ship the runnable bits in the SDK, we ship 
> those on the device updates).


This would require creating a linker for these files. Is this what you intend 
to do?

> 2. -emit-ifso could be added to whatever the existing set of build flags are 
> to be run as a pre-build step to produce tapi-like .so files that break up 
> build dependencies.

How would this work? To be clear, this would happen at the same time that .o 
files are produced correct? The build graph would basically look the same but 
your new linking step would replace the original link step and there would be a 
pendent node for the original link step. This would not break up the build 
graph much at all save for the fact that we might imagine that this new link 
step would be much faster.

> 3. -emit-ifso -fvisibility=hidden could added to the invocation to disallow 
> usage of non-public apis.

Can you be more specific? This is already how the actual modules do this today 
so what is the advantage here.

> The way we are thinking it would work is similar to the dwos and .o files 
> where you get one .ifso for each TU, then have a link-action that merges the 
> mangled names and produces the .so file.

That requires a linker but one that only has to link dwarf which is designed to 
be linked using the dumbest possible semantics so that it can be format 
agnostic and not create undo complexity in the linker. The linker needed for 
this sort of thing would have to understand almost everything that a real full 
linker has to just to determine if a symbol is public or not. I think this 
might even require processing relocations. Beyond being public symbols have 
extra information that matters at both static and dynamic link time. This is 
mostly just point out how complicated this task is and not a complaint about 
the current output format. That could be improved but we have to be aware of 
the complexity of introducing yet a third type of linker and the issues with 
maintaining parity with the behavior of existing linker as well. In particular 
I think introducing a linker is an even bigger llvm-dev discussion that we need 
to have before we should proceed with this method.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

I'm not misunderstanding, please seem my post on llvm-dev where I layout the 
different issues involved. You're underestimating the complexities involved 
here I think. Trivializing what the linker does to "the linker just merges 
these" is a gross oversimplification that doesn't account for symbol consuming 
vs exporting, size, alignment, visibility, linkage, symbol type, symbol 
versioning (which is impossible to handle here without consuming version 
scripts as was pointed out by someone else), which symbols are untimely 
exported/consumed or even intermediate operations on .o files via tools like 
objcopy (which is uncommon and shouldn't be a hard blocker here but something 
to think about). Also all linkers behave just a bit differently with respect to 
these issues so 1) the reality is that the thing that merges these will behave 
just a bit differently than any existing linker but 2) even if it trys to match 
one linker it will fail to match the other so it will only be compaitable in 
all cases with atmost one linker (but probably none when edge cases are 
considered. I also already pointed out the issue with having to search all 
source files on the tree. I agree that if you were to use the linker to 
accomplish this that would would have to look at all source files. Further more 
many flags passed to the linker untimely affect these things making matters 
more complicated. The "linker" you describe for these cases is far from "cat".

For reference when I say "module" in this context I mean the output of the 
linker. "DSO" is synonymous. A DSO/module can be an executable or shared object 
but we can consider both here. I was not referring to C++ modules.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

Why is this better than producing output from the output of the static linker 
at the end? Also how do we plan on integrating this with llvm-elfabi so that we 
don't have unnecessarily duplicated efforts? Why does that tool not suffice?

> I'm well versed in the complexities of a linker - I've worked extensively on 
> the GNU linkers as well as with lld. The visibility of the symbols is 
> actually computed by the compiler - the STV_* flags associated with the 
> symtab entry give you that information which is actually tracked through the 
> frontend to the backend. Yes, each linker behaves differently - including lld 
> which completely changes the semantics of Unix/ELF linking guarantees. In 
> fact every single attribute that you mentioned is something which is emitted 
> from the compiler - which we have access to here given that we are in clang 
> itself. I think that this can be made to work properly, though will probably 
> require some iteration to get all the corner cases, and that you may be 
> slightly dismissive of this approach.
> 
> Ah, okay, I didn't pick up that you were using module as the actual module 
> from the context since normally the module doesn't control what it exposes 
> itself.
> 
> Note that I am not advocating that this change go in as is - I think that 
> this is far from something which would be useful, and until it can produce 
> something meaningful, this shouldn't really be merged (i.e. generate 
> something which obj2yaml or some other tool can consume to generate the 
> barest ELF stub).

It's not a matter of where they're produced, its how you treat them and ensure 
that they're correctly handled so that the final produced stub has the correct 
output. I don't think this is entirely trivial when you consider all of those 
different aspects that I listed.

> No, I do not intend to write any sort of linker in the classical sense. At 
> most I'd be handling and merging symbol names from yaml files or possibly 
> going from yaml -> elf .o -> .so. Weighing options.

I'm calling that a kind of linker here. I'm not saying it would be as 
complicated as a full linker but it would be a linker. I am claiming it would 
be more complicated than the dwp linker however. Whatever you're calling it you 
have to write it and that's an important part of this proposal process but that 
wasn't mentioned until I asked about it.

> I was thinking that since -emit-ifso doesn't invoke the backend, it could be 
> run as a pre-build step. Then during the build, linking is done against the 
> ifso stubs that are already generated instead of the actual .so files.

That seems a lot better than what I understood. This behavior might be useful 
to some people. In particular you don't need .tbe files in your source tree so 
if that isn't something you want to have this would be a less ideal alternative 
because it was to invoke enough of the compiler to get rich symbol information 
but not so much that its as slow as compiling. You pay for this by having to 
reparse and go though some of the compilation process and you have to 
link/merge many files to produce the stub but it doesn't require funny files in 
your tree. I think that's a niche that some people might want. The tradeoff 
between not having to deal with .tbe files and the huge increase of build 
steps, computation needed (overall build times could very well decrease if your 
build wasn't sufficiently parallel due to linking orders), and total amount of 
IO seems like a tradeoff that I personally wouldn't want to make. Is this a 
trade off you want to make? Almost every metric I can think of would be worse 
than just adding .tbe files into your source code.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

> I actually don't see why something like this couldn't be used in both 
> scenarios (ie some users generating the files at build time, and others using 
> -emit-ifso to periodically generate and land changes to checked in files).

They could be used in parallel in most cases but they would likely have 
divergent behavior unless we shared the writers and there be duplicated code. 
This is not strictly a bad thing and efforts to merge them might not be 
fruitful or ideal but its something I think we should discuss. A real 
possibility is that llvm-elfabi could be the linker than merges these since it 
already has a carefully thought out and evolving internal representation of an 
ELF file and will soon have readers/writers for both stubs and .tbe files.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

> The strong motivation here is to avoid having the list be maintained outside 
> of the source code. I am not too strongly tied to the YAML approach as long 
> as the binaries that we get at the end are equivalent to the pruned ELF 
> binaries from the YAML2ELF approach. However, the trade off here is valuable 
> - it allows for the source code to be the source of truth for the interface 
> set. The intermediate emission is due to the fact that this requires program 
> level visibility and having the intermediates an extra step to merge is 
> relatively simpler. In fact, you could emit the TBE files from this and check 
> that into the tree if you really wanted (but allowing the source to remain 
> the source of truth I think is a valid and valuable trade off). The reason 
> for the selection of YAML is simply pragmatism - that is already available in 
> the tree. If the TBE linker is available now, I have no issue with that being 
> the intermediate representation (assuming that the representation is easy to 
> parse programatically).

You can verify that a .tbe file is consistent with the finally fully linked 
executable to ensure that it is consistent with the actual source of truth, the 
finally linked binary (the source code is not really the source of truth on 
this since compiler and linker flags alter these things). This is a planned 
feature of llvm-elfabi. Keep in mind that having .tbe files in tree means that 
you can also review the public interface of modules exactly as a part of code 
review which your method allows for only in the traditional way. When you 
consider that compiler and linker flags can change these things there is no one 
single source of truth that humans can easily look at without something like 
the .tbe files. This was a major motivation behind the choice here.

The tbe format is not suitable as a pre-merged format I think. I propose that 
you could take your pre-merged format and do the merging in llvm-elfabi (or 
rather, the tool would be a symlink to the same binary with a different name) 
to produce the same internal representation that llvm-elfabi already uses and 
then you could use the same writers for both .tbe and ELF stubs. This would 
allow sharing of the relevant code and would trivially allow both .tbe files 
and stubs to be produced. Also, as a side note, .tbe files are also YAML for 
the same reason you're using it here. I don't have an issue with that.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

To be clear I'm only really giving my thumbs up to a specific high level plan 
that I believe handles a case that llvm-elfabi wouldn't otherwise handle on its 
own. I consider the specifics like the output format still in the air right 
now. I strongly believe that a unique yaml or json format should be used for 
this. We should discuss the specifics of what should go in that format and have 
a discussion on that format. The trick is to capture everything in .o files 
that could wind up mattering to a .tbe file and nothing more. I haven't thought 
though what that entails completely yet but I'll think about it and throw up my 
proposal. Once we have at least one proposal we can start iterating on the 
specifics of it.

Here's the plan I think we can converge which at least covers all of my 
concerns:

1. A flag is added to clang that would cause clang to emit a pre-merged format 
and not invoke the backend
2. The pre-merged format can then be merged into a stub or .tbe file as part of 
the same binary (but an effectively different tool since a symlink and options 
parser will be used instead) as llvm-elfabi
3. The yaml code for this format will go in llvm so that both clang and the new 
merging tool could use it
4. We need to discuss the specifics of the pre-merged format to ensure that it 
captures everything that matters (and hopefully nothing more)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

In D60974#1477690 , @compnerd wrote:

> @jakehehrlich - when do you expect to have your idea put up?  I don't think 
> that it is fair to have this wait until you have time to put something up 
> that can be discussed.  I think that getting this working and then iterating 
> on it and migrating it over to some shared representation is something which 
> we could do - that tends to be a common thing that I have seen happen 
> multiple times with the necessary work never materialising.  Re-use of the 
> YAML structure means that we can iterate and identify the pieces that are 
> necessary, though, I expect that largely, what will be needed is the name, 
> the binding, the visibility, possibly the size (for TBEs), the section, and 
> the type, at least for anything which adheres to the GABI.  If you have 
> extensions outside of GABI, this will need to be adjusted.


I don't know when but in the next 2 days likely. Regardless of my timeline I 
don't think I'm blocking anyone. I'm just saying that I will put up a proposal. 
You should also put up a proposal as well. The yaml2obj format is just not well 
designed for any purpose but is far from designed for this purpose. yaml2obj 
works ok-ish for testing. I'd rather start with a minimal format and then add 
things to it rather than starting with a bloated and ill designed format and 
then create a new format from that experience. Creating a minimal format 
shouldn't be hard and I suspect our proposal will look extremely similar; I'd 
wager if you put up a proposal I'd probably just review your proposal rather 
than bother writing out my own.

As for what I think it would entail. I think name, weather or not the symbol is 
defined or undefined, visibility, size, alignment (this is a feature of the 
section and the symbol offset) and type will all mater but not all combinations 
make sense. Sections don't matter as it turns out but alignment does for copy 
relocations. When we started llvm-elfabi I thought at least the section 
permissions mattered but they don't really. While .tbe has things like soname 
and dt_needed that isn't needed here.  The top of the file should probably 
contain the architecture. Other details in the ELF header shouldn't be needed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

There seems to be some confusion about the term "format" here. There's the YAML 
format but within that there are specific YAML formats. My proposal will be to 
have a specific YAML format that just isn't the one used by yaml2obj. YAML 
isn't the issue, it's the structure that that tool specifically uses.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

As an example, .tbe and .tbd files use YAML but in a very specific well defined 
structure. yaml2obj also uses a specific format which is actually well defined, 
it just doesn't map on very well here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

Why would you convert the actual format, which is already using YAML, to the 
yaml2obj format? That seems to have zero gain to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

I think I know the format that I'd like *roughly*. I haven't worked out how to 
handle COMDAT and section groups yet. Roland seems to think that COMDAT and 
section groups will be an issue but I'm working out the details with him to 
make sure I understand it. I'll get back to you soon.

  Version: 1.0
  Arch: AArch64
  Symbols:
- Name: foo
  Type: Function
  Weak: true
- Name: bar
  Type: Function
  Defined: false
- Name: baz
  Type: Object
  Size: 256
  Alignment: 32

Only Arch is used other details from your header can be inferred as defaults 
but are not needed here. I think binding is not needed beyond Weak vs. Not 
Weak. I spoke with Roland about this and he pointed out STB_GNU_UNIQUE. This 
would motivate allowing Global, Weak, and GNU_UNIQUE as valid bindings. The 
only cases for viability that we care are protected and default. Internal and 
hidden will never appear in the public interface. We can treat protected and 
default as the same as they just mean that the symbol will appear in the public 
interface and their difference is related to how the linker resolves symbols 
relative to module itself not other external modules.

We don't have to worry about symbol version (and the TextAPI types don't 
support that yet either) but Warnings are another case to think about. Each 
symbol can have a warning string associated with it. This will be one of the 
last features (before symbol versions but after almost everyhing else) that 
support is added for in llvm-elfabi so I think we can ignore it here for now.

One thing we forgot to add in llvm-elfabi that we now know we need to add is 
Alignment. Alignment can be computed from the section and the offset within the 
section. This is the one bit of information that comes from the section itself 
and not the symbol table.

Absolute symbols probably aren't needed but if we ever need them they're easy 
enough to add via a new symbol type.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

In D60974#1483164 , @plotfi wrote:

> So, @compnerd and I have discussed and we'd like to propose a yaml schema 
> that can express what we need for ifsos for the Sections and the Symbols.
>
> ...


Can you clarify is this is for the fully merged format or for the unmerged 
format? I *really* strongly believe that the fully merged format should go 
though llvm-elfabi.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

To clarify my format is for the unmerged format. The merged format should be 
the ELFStub internal representation found in llvm's TextAPI. The plan is to 
have at minimum .tbe and ELF stubs. I'm actively working on getting the ELF 
stub output which can then be converted to the yaml2obj format since they will 
just be ELF files.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

In D60974#1483240 , @plotfi wrote:

> Me and @compnerd had discussed a more abstracted format like this but decided 
> it was best to just use the same names that are in the ELF already.
>  Is there any compelling reason not to?
>  As far as I understand, by having something like "Weak: true" is already 
> harking back to ELF so why not stick to the same names?
>
> I think the !tbd-elf-v1 versioning can help with any changes or alterations 
> we want to make along the way too.
>  We did discuss the alignment field too.


The format will have to be ELF specific but that doesn't mean we have to use 
the exact names. The benefit of this format is that you can only do the 
intended thing with it while anything more. This is also the format that 
matches most closely with .tbe which is a plus for consistency of this and 
integration of both tools into the llvm ecosystem. It's obvious how to merge my 
format into the ELFStub format. Your format has extraneous details that would 
never matter to the creation of the ELFStub format like the name of the section 
a symbol was defined in. Also I think much more of the compiler has to be 
considered to get section names right unless you're just recomputing them and 
then that's redundant for no gain.

When we first reviewed the .tbe format I was on the "just use ELF name" side of 
things for the Type and Arch fields. I don't remember why that wasn't chosen 
but for consistency with that choice I think we should do the same thing here. 
As for fields like Weak vs not Weak distilling the format down to only allow 
for those options is a design benefit. not Weak means (GLOBAL or GNU_UNIQUE) 
and Weak means WEAK. Likewise by not messing with visability (you don't mess 
with it either, I'm just pointing this out) we split things into (default | 
protected) and (internal | hidden). These grouped concepts, though exactly what 
we need, are not what available concepts. We also constrain useless bindings 
from existing like LOCAL which isn't meaningful in this context.

Every bit of my format is needed to produce the final stub in working form. All 
section information in your format, some bindings, and ELF file type are all 
not needed. There's an argument for embedding the ELF encoding format in this 
file but that will only be used later once linking occurs. It can also be 
inferred by the architecture in most cases and otherwise specified using a bfd 
output format name when that's what people need/want.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

> I think the !tbd-elf-v1 versioning can help with any changes or alterations 
> we want to make along the way too.

I think this is good. I kind of glanced over it and mentioned a Version field. 
Version is handled explicitly by a VersionTuple in llvm-elfabi. I think we 
should do the same thing here. I forget all the details of how this works but 
certainly versioning is a good idea.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added inline comments.



Comment at: clang/include/clang/Driver/Types.def:91
 TYPE("ast",  AST,  INVALID, "ast",   "u")
+TYPE("ifso", IFSO, INVALID, "ifso",  "u")
 TYPE("pcm",  ModuleFile,   INVALID, "pcm",   "u")

I think a different name should be used for a few reasons

1) ifso is the name of a tool used internally at Google that isn't anything 
like this. I'd like to avoid confusion or anything related.
2) .tbe is the format name for the textual fully linked format in llvm and I 
feel like we should hearken back to this but this isn't critical really.
3) the end is "so" but this is the intermediate format should it should not 
have the "so".

I think ".tbo" or ".tbe.o" or something like that would be my preferred sort of 
name. This is also in holding with ".tbd" which is the text based version of 
".dnylib" on MacOSX. so it's generally consistent.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

In D60974#1483380 , @plotfi wrote:

> Also, curious on the lack of denoting the sections and which symbols go in 
> which sections? Do you just assume that "Type: Object" goes into .data?


You don't need that information and the linker doesn't use it. It only uses 
that information to get alignment. In the code that I'm writing right now it 
happens to put everything in a single rodata section (except for tls which it 
puts in a TLS rodata section, which isn't a thing that even exists in the wild)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

In D60974#1483399 , @plotfi wrote:

> In D60974#1483265 , @jakehehrlich 
> wrote:
>
> > In D60974#1483240 , @plotfi wrote:
> >
> > > Me and @compnerd had discussed a more abstracted format like this but 
> > > decided it was best to just use the same names that are in the ELF 
> > > already.
> > >  Is there any compelling reason not to?
> > >  As far as I understand, by having something like "Weak: true" is already 
> > > harking back to ELF so why not stick to the same names?
> > >
> > > I think the !tbd-elf-v1 versioning can help with any changes or 
> > > alterations we want to make along the way too.
> > >  We did discuss the alignment field too.
> >
> >
> > The format will have to be ELF specific but that doesn't mean we have to 
> > use the exact names. The benefit of this format is that you can only do the 
> > intended thing with it while anything more. This is also the format that 
> > matches most closely with .tbe which is a plus for consistency of this and 
> > integration of both tools into the llvm ecosystem. It's obvious how to 
> > merge my format into the ELFStub format. Your format has extraneous details 
> > that would never matter to the creation of the ELFStub format like the name 
> > of the section a symbol was defined in. Also I think much more of the 
> > compiler has to be considered to get section names right unless you're just 
> > recomputing them and then that's redundant for no gain.
>
>
> We wanted to use the same names because its just a lot easier understand what 
> is if you've already looked at the ELF header code (ie STT_FUNC vs Function).


This is a reasonable opinion and was my opinion as well. But that isn't the way 
review went for .tbe and so now we have a responsibility to be consistent. This 
is bike shed level stuff. I could care less either way except for consistency.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

With respect to group handeling. I think we should just check that the details 
of every symbol across all unmerged files that are being merged are consistent 
and then ignore duplicates.

For symbols that are not defined in sections that are a part of a comdat group 
this is just delaying the error to the linker. For symbols defined in a comdat 
group this is the only way we can be consistent with whatever choice the linker 
makes. In practice every comdat group is the same but if it were different the 
linker is allowed to just pick one arbitrarily. The only way to be consistent 
is to ensure that duplicates are identical. This however means we ignore 
duplicate definitions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

> Does llvm-elfabi consume your proposed Schema format? Has it landed yet?

No, I just proposed it and explained my reasoning. If you wanted to add this 
format to TextAPI that would be acceptable.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

Background:
There are two parts to this. TextAPI is a public interface of textual 
representations for these sorts of files that already exists in llvm. There 
exist ELF and MachO textual representations currently. llvm-elfabi is a tool 
for 1) Creating Stubs 2) Creating .tbe (the TextAPI fully linked ELF format) 
files 3) Converting between them and 4) Analyzing the details of the formats.

You asked about the format I proposed here, not the already accepted .tbe 
format. The format proposed here could be added to TextAPI where it could 
subsequently be used in both Clang and a tool that would use the llvm-elfabi 
code to create stubs. I imagine the flags needed for this new tool would be 
very differnet from what llvm-elfabi uses or would use in the future so we'd 
probably want a second symlink the behaved very differently.

It can currently read ELF and .tbe files and can currently write .tbe files. 
I'll have the basics of ELF writing up for review by the end of the day and 
that will, for the next month or two be my primary work which should likely see 
it close to stabilized. I'd expect landing the basics of ELF writing to take 
another week or two since its somewhat involved and I'll likely need to split 
the patches up from what I have. Given that no such writer currently exists 
this should put a version of your tool well ahead of schedule. Such are the 
wonderful benefits of open source.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

Well I think it should be a seperate tool but because the code inside 
llvm-elfabi isn't a library yet (though it is written to be used like one) we 
can do that by adding a symlink and changing the behavior of the underlying 
binary to present as a different tool. Many different tools around llvm do this 
for similar reasons.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

The linker doesn't look at section permissions. In fact the only thing it even 
looks at the section index for is to check for alignment for copy relocations 
which is something most people don't even use.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

> Wanted to mention, from my testing with yaml2obj, I needed the section flags 
> (SHF_EXECINSTR, SHF_WRITE, SHF_ALLOC, etc) otherwise nm was spitting the 
> wrong thing. How are you going to handle those?

I'm not sure what you mean. yaml2obj is a different tool that we shouldn't be 
using IMO. yaml2obj would require the sections to denote that a symbol was 
defined. When producing an elf file from the .tbe format you have to synthesize 
sections for this same reason.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

Oh and "ifo" is a good name.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

In D60974#1486384 , @compnerd wrote:

> @jakehehrlich - unfortunately, removing the attributes on the sections will 
> make the content look different with `nm` which is something we do need to 
> appear proper for consumers of the interface libraries.  Versioning has a 
> number of problems inherent to it (unfortunately, its the closest to 
> multi-level namespaces in ELF).  I think that getting something in tree and 
> iterating on it is far better than continuing to argue over this in the dream 
> of something that unifies the TAPI approach and this approach.  The section 
> names are relevant (you can add attributes to put symbols into alternate 
> sections and you can have section relative relocations).  I think that you 
> are loosing fidelity in the final output which is sufficient for your needs, 
> but I think that there are places where the full fidelity can be needed.
>
> This currently works and allows us to generate the interface library which 
> means that this is actually further than what you are proposing still.  Is 
> there something technical that this is doing incorrectly or breaking 
> something?  Otherwise, this really does seem like it is devolving into a bike 
> shedding argument that isn't really going anywhere.  This is not a large 
> amount of code and there is backing to maintain it, so it is not an issue of 
> "this is adding un-maintained complexity" either.
>
> Just like the LLVM APIs, this can/will evolve.  I don't see why this needs to 
> be set in stone from the initial implementation.  There are use cases which 
> can come up which require reworking the solution.




1. The output of nm when you give it what exactly is different? What output are 
you expecting? You aren't making that specific. Passing the unmerged output 
though nm doesn't make any sense. If you have a precise use case for section 
names you should mention it. We can always add symbol information back as 
needed if we first start with something more minimal. What output of nm are you 
expecting a

2. We're basically in agreement on what should happen here. I don't think it's 
a dream of unifying these things. It looks like a very close reality to me.

3. You mention sections and relocation as being relevant information. Can you 
give a specific use case? Section names in general are not meaningful 
information. They're just there for humans to debug things. Putting symbols in 
different sections lets the linker treat them specially but in the end binary 
sections are not relevant to much of anything.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

> Jake, I am still not sure what you would prefer for moving forward. The 
> current patch can output the yaml format I originally proposed or the one 
> that looks similar to the one you proposed. What I need to know is:
> 
> Do you want the merging if the "ifo" files to happen inside of llvm-elfabi?
>  Do you care if we upstream the yaml we proposed as a first step (which is 
> really a distilled version of that yaml2obj can consume anyways. this right 
> now functions actually) ???
>  Or, would you rather the ifo files be merged by a different separate tool 
> (something maybe called llvm-ifsogen)???

This is my proposal:

1. We should plan on adding the code ofr merging these files inside of 
`llvm/tools/llvm-elfabi` but will it be a separate tool from `llvm-elfabi`. You 
can create "separate" tools in the same directory using the symlink trick. See 
llvm-ar and friends or llvm-objcopy and llvm-strip. The tool name can be 
discussed in review.
2. The yaml proposed thus far is not acceptable IMO for reasons already 
outlined but this can be discussed in code review. Before adding sections a 
clear reason for needing them is required which hasn't been given yet. Things 
can evolve and change later as needed but we should start with the minimum and 
add as needed later; not start with extra and remove later. Support for the new 
format should be added into TextAPI and in the review for that process we 
should discuss the format. After we add support for this new format into 
TextAPI we can add support for emitting this format into clang (well actually 
you can write the code whenever but I'm using "after" in a different sense 
here).
3. After support for emitting this is in clang has landed you can write the 
merging tool using the symlink trick. The merging tool should read in all the 
.ifo files and generate an ELFStub (see TextAPI/ELF) which can then be output 
using the code that I recently added you as a reviewer on. This will ensure 
that all of the stub solutions in llvm are consistent.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62970: [clang-doc] De-duplicate comments

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added inline comments.



Comment at: clang-tools-extra/clang-doc/Representation.cpp:124
+  for (auto &Comment : Other.Description) {
+bool IsCommentUnique = std::find(Description.begin(), Description.end(),
+ Comment) == Description.end();

juliehockett wrote:
> ```if (std::find(Description.begin(), Description.end(), Comment) == 
> Description.end())
>   Description.emplace_back(std::move(Comment));```
Instead of deduping like this can we just make Description a set?



Comment at: clang-tools-extra/clang-doc/Representation.h:60-65
+for (unsigned I = 0; I < Children.size(); ++I) {
+  if (!(*Children[I] == *Other.Children[I]))
+return false;
+}
+return true;
+  }

You can do this using llvm::make_pointer_range/llvm::pointer_iterator and 
std::equal


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62970/new/

https://reviews.llvm.org/D62970



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62970: [clang-doc] De-duplicate comments

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

Actually if we can make Description a hash set that would be best. I think 
using std::set would require an awkward < operator to be defined. There should 
be standard ways of getting hashes out of all the string and vector types and 
you can use hash_combine to combine all of these into a single hash.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62970/new/

https://reviews.llvm.org/D62970



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64539: [clang-doc] Add stylesheet to generated html docs

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

I'm blindly accepting the CSS but all other code looks fine. I still have that 
chrome bug where I can't LGTM sometimes but consider this reviewed and accepted 
by me as well


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64539/new/

https://reviews.llvm.org/D64539



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65627: [clang-doc] Continue after mapping error

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

What's the use case for this and how do we handle return codes when an error 
occurs but we continue on?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65627/new/

https://reviews.llvm.org/D65627



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64958: [clang-doc] Fix link generation

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich accepted this revision.
jakehehrlich added a comment.
This revision is now accepted and ready to land.

The code looks fine to me but I don't understand the global details of this. 
Hopefully Julie can still review things from there but if not we don't get 
timely reviews then we can still land this.




Comment at: clang-tools-extra/clang-doc/Representation.h:140-141
+  llvm::SmallString<128>
+  Path; // Path of directory where the clang-doc generated file will be
+// saved (possibly unresolved)
+  bool IsInGlobalNamespace =

Here and below perhaps favor putting the comments above the line rather than to 
the side to make the formatter produce nicer looking code.



Comment at: clang-tools-extra/unittests/clang-doc/YAMLGeneratorTest.cpp:83-84
   I.TagType = TagTypeKind::TTK_Class;
-  I.Parents.emplace_back(EmptySID, "F", InfoType::IT_record, "path/to/F");
+  I.Parents.emplace_back(EmptySID, "F", InfoType::IT_record,
+ ""); // F is in the global namespace
   I.VirtualParents.emplace_back(EmptySID, "G", InfoType::IT_record,

Maybe put the comment above this line so that the formatter doesn't get all 
wonky.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64958/new/

https://reviews.llvm.org/D64958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65003: [clang-doc] Add index in each info html file

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

I think everything but the implementation of `genIndex` being confusing looks 
good to me. I haven't actually groked how that code works yet other than the 
fact that it generates the index tree that I expect and all the surrounding 
code looks good to me. I understand that some of the trickiness here comes from 
the fact that you're building from a list of values but trying to generate the 
tree structure from that list which is hard. I think we can structure that code 
better; lets see if we can't device a better algorithm.




Comment at: clang-tools-extra/clang-doc/Generators.cpp:16
 
+Index Generator::genIndex(const std::vector> &Infos) {
+  Index Idx;

Please document this function with more internal comments. I have no idea 
what's going on here and the shifting of 'I' is super confusing to me.



Comment at: clang-tools-extra/clang-doc/Generators.cpp:20
+Index *I = &Idx;
+for (auto R = Info->Namespace.rbegin(), E = Info->Namespace.rend(); R != E;
+ ++R) {

Use llvm::reverse https://llvm.org/doxygen/STLExtras_8h.html


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65003/new/

https://reviews.llvm.org/D65003



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65030: [clang-doc] Add second index for sections within info's content

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich accepted this revision.
jakehehrlich added a comment.
This revision is now accepted and ready to land.

This looks awesome!




Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:257
+genTypeReference(const Reference &Type, StringRef CurrentDirectory,
+ StringRef JumpToSection = "") {
+  if (Type.Path.empty()) {

Use an optional StringRef instead


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65030/new/

https://reviews.llvm.org/D65030



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65483: [clang-doc] Add link to source code in file definitions

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added inline comments.



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:376
+  LocNumberNode->Attributes.try_emplace(
+  "href", (FileURL + "#" + std::to_string(L.LineNumber)).str());
+  Node->Children.emplace_back(std::move(LocNumberNode));

Add a comment here that this is the github/googlesource notation for this that 
way in the future people arren't confused when it doesn't work for other 
hosting pages



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:377-378
+  "href", (FileURL + "#" + std::to_string(L.LineNumber)).str());
+  Node->Children.emplace_back(std::move(LocNumberNode));
+  Node->Children.emplace_back(llvm::make_unique(" of file "));
+  auto LocFileNode = llvm::make_unique(

Can you put these in the link so that the link is larger than a single number? 
e.g. "303 of file foo.c" should be linkified rather than just "303" which is 
how I read the code now.



Comment at: clang-tools-extra/clang-doc/Mapper.cpp:103-104
+  llvm::SmallString<128> Prefix(RootDir);
+  if (!llvm::sys::path::is_separator(Prefix.back()))
+Prefix += llvm::sys::path::get_separator();
+  llvm::sys::path::replace_path_prefix(File, Prefix, "");

Do you actually need to do this? Feels like a bug in replace_path_prefix per 
its own documentation if you do.



Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:228
+  RepositoryUrl.find("https://";) != 0)
+RepositoryUrl.insert(0, "http://";);
+

1) Do we need to add this for the user?
2) Can we use https by default if we need this?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65483/new/

https://reviews.llvm.org/D65483



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65628: [clang-doc] Parallelize reducing phase

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

LGTM other than a couple atomicity issues.




Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:244
+  llvm::errs() << toString(ReadInfos.takeError()) << "\n";
+  Error = true;
+  return;

use std::atomic



Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:262
+llvm::errs() << toString(InfoPath.takeError()) << "\n";
+Error = true;
+return;

This isn't technically thread safe. Use an std::atomic


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65628/new/

https://reviews.llvm.org/D65628



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65622: [clang-doc] Update documentation

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich accepted this revision.
jakehehrlich added a comment.
This revision is now accepted and ready to land.

This looks fine to me


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65622/new/

https://reviews.llvm.org/D65622



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62970: [clang-doc] De-duplicate comments and locations

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

LGTM




Comment at: clang-tools-extra/clang-doc/Representation.h:66
+
+  bool operator<(const CommentInfo &Other) const {
+auto FirstCI = std::tie(Kind, Text, Name, Direction, ParamName, CloseName,

We should be explicit about what we intend to happen here. Do we just want a 
total ordering of any kind? Do we want some things to be "more important" than 
others? For instance because the ordering is lexicographical here Kind is more 
important than Text which is more important than Name etc...

If the intent is just to have any total order state so at the top and why we 
need a total ordering (for instance to have an std::set/std::map of these 
things or we need to sort them, etc...)



Comment at: clang-tools-extra/clang-doc/Representation.h:186
 
+  bool operator<(const Location &Other) const {
+return std::tie(LineNumber, Filename) <

Same comment here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62970/new/

https://reviews.llvm.org/D62970



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63180: [clang-doc] Adds HTML generator

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

I got to the 'genHTML' functions and I decided that this general approach is 
not great.

What if we have a baby internal representation of an HTML tree, generate 
instances of that, and then implement a render method on that (preferably one 
that handles indentation or would have the ability to do so soon).

I also think we could split this up and support only a subset of the comment 
types at first to reduce the review load.




Comment at: clang-tools-extra/clang-doc/Generators.cpp:60-71
+std::string genReferenceList(const llvm::SmallVectorImpl &Refs) {
+  std::string Buffer;
+  llvm::raw_string_ostream Stream(Buffer);
+  bool First = true;
+  for (const auto &R : Refs) {
+if (!First)
+  Stream << ", ";

This seems to be displaying a list in a particular way. Mind adding a comment 
about where this is used?



Comment at: clang-tools-extra/clang-doc/Generators.cpp:65
+  for (const auto &R : Refs) {
+if (!First)
+  Stream << ", ";

You can just use `&R != Refs.begin()` or `&R != &Refs.front()` and then you 
don't have to keep any local state and its clear when that condition would be 
true/false.



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:25
+
+std::string genTag(const Twine &Text, const Twine &Tag) {
+  return "<" + Tag.str() + ">" + Text.str() + "";

This can also be a Twine



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:33-35
+void writeLine(const Twine &Text, raw_ostream &OS) {
+  OS << genTag(Text, "p") << "\n";
+}

I'm not familiar with HTML.  What does this '' do?



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:66
+OS << genEmphasis(I.ParamName) << I.Text << Direction << "\n\n";
+  } else if (I.Kind == "TParamCommandComment") {
+std::string Direction = I.Explicit ? (" " + I.Direction).str() : "";

Instead of repeating code add an "||" case to the above.



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:67-68
+  } else if (I.Kind == "TParamCommandComment") {
+std::string Direction = I.Explicit ? (" " + I.Direction).str() : "";
+OS << genEmphasis(I.ParamName) << I.Text << Direction << "\n\n";
+  } else if (I.Kind == "VerbatimBlockComment") {

Instead of making a local `std::string` you can first write the emphasis then 
decide wheater or not to output the direction, and then output the newlines 
unconditionally.



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:72-77
+  } else if (I.Kind == "VerbatimBlockLineComment") {
+OS << I.Text;
+writeNewLine(OS);
+  } else if (I.Kind == "VerbatimLineComment") {
+OS << I.Text;
+writeNewLine(OS);

Dedup these.



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:81-84
+std::string Buffer;
+llvm::raw_string_ostream Attrs(Buffer);
+for (unsigned Idx = 0; Idx < I.AttrKeys.size(); ++Idx)
+  Attrs << " \"" << I.AttrKeys[Idx] << "=" << I.AttrValues[Idx] << "\"";

I don't see the need for an intermediete ostream. 

```
OS << "<" << I.Name;
for (...)
  OS << ...;
writeLine(I.SelfClosing ? ..., OS);
```

would work fine and be more efficient.



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:86
+
+std::string CloseTag = I.SelfClosing ? "/>" : ">";
+writeLine("<" + I.Name + Attrs.str() + CloseTag, OS);

This can be a StringRef



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:106-107
+
+  std::string Buffer;
+  llvm::raw_string_ostream Members(Buffer);
+  if (!I.Members.empty())

This is a generally bad pattern.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63180/new/

https://reviews.llvm.org/D63180



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63857: [clang-doc] Structured HTML generator

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

Overall looks better. One of my comments causes a sweeping change to occur so 
I'll respond back after thats done.




Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:91
+struct HTMLFile {
+  llvm::SmallString<16> DoctypeDecl{""};
+  std::vector> Children; // List of child nodes

Does this ever change? If not this should be a global constexpr constant char* 
inside of an anon-namespace.

e.g.

namespace {
  constexpr const char* kDoctypeDecl = "...";
}



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:170
+OS << " " << A.getKey() << "=\"" << A.getValue() << "\"";
+  OS << (SelfClosing ? "/>" : ">");
+  if (!InlineChildren)

You can exit here if its self closing right?



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:187
 
-std::string genTag(const Twine &Text, const Twine &Tag) {
-  return "<" + Tag.str() + ">" + Text.str() + "";
+static void genHTML(const EnumInfo &I, TagNode *N);
+static void genHTML(const FunctionInfo &I, TagNode *N);

You should return TagNodes, not assign to them by ref. Preferably via retruning 
a unique pointer. It doesn't make sense to have the consumer do the constructor 
I would imagine. For instance what tag is the right tag type? The consumer 
shouldn't decide, the producer should. 



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:190
+
+static void genEnumsBlock(const std::vector &Enums, TagNode *N) {
+  if (Enums.empty())

Same comment here these function should look like (at a very high an imprecise 
level, obviously some functions will be more complicated.)

```
std::unique_ptr genFoo(info) {
  auto out = llvm::make_unique<...>(...);
  for (const auto &B : info.bars)
out->Children.emplace_back(genBar(B));
  return out;
}
```

This goes for all of these functions


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63857/new/

https://reviews.llvm.org/D63857



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63180: [clang-doc] Adds HTML generator

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

I'm getting confused in all the different patches. I reviewed the HTML 
generator one like yesterday and was awaiting changes. Doesn't this do the same 
thing?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63180/new/

https://reviews.llvm.org/D63180



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63857: [clang-doc] Structured HTML generator

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

Can you upload `git diff -U10 ` functions so that I 
can see the code without it being fragmented? It's hard to parse some of the 
more fragmented stuff here




Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:191-194
+static std::vector> genHTML(const EnumInfo &I);
+static std::vector> genHTML(const FunctionInfo &I);
+
+static std::vector>

Never use 'static' for functions in C++. For functions anon-namespaces and 
'static' mean the same thing but C++ generates a lot of stuff behind the scenes 
and to make those 'static' you need to use an non-namespace.

In general the rule is to put as much as you can in anon-namespaces and always 
prefer that for functions/globals/constants over 'static'. This is important 
for `constexpr` variables specifically because depending on the C++ version 
they don't play nice with the `static` keyword.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63857/new/

https://reviews.llvm.org/D63857



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63857: [clang-doc] Add a structured HTML generator

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added inline comments.



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:228
+  Out.emplace_back(llvm::make_unique(HTMLTag::TAG_H2, "Functions"));
+  Out.emplace_back(llvm::make_unique(HTMLTag::TAG_DIV));
+  for (const auto &F : Functions) {

Refering to this as `Out.back()` is confusing, can you give it a variable name? 
It took me longer than I care to admit to figure out that you weren't appending 
to `Out` in the loop. A pretty common pattern I use is to just assign `back()` 
to a reference that I'm going to use a lot.

```
Out.emplace_back(...);
auto& DivBody = Out.back();
```

That way you get the inplace construction and you can refer to it wherever 
you'd like by name.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63857/new/

https://reviews.llvm.org/D63857



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63857: [clang-doc] Add a structured HTML generator

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

This looks good to me!




Comment at: clang-tools-extra/clang-doc/Generators.cpp:75-77
 extern volatile int YAMLGeneratorAnchorSource;
 extern volatile int MDGeneratorAnchorSource;
+extern volatile int HTMLGeneratorAnchorSource;

I don't fully understand these lines. Can you explain how they work and what 
they accomplish?




Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:75
+Children.emplace_back(
+llvm::make_unique(Text.str(), !InlineChildren));
+  }

I think you can just write `Children.emplace_back(Text.str(), !InlineChildren)`



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:310-312
+std::unique_ptr Node = genHTML(Child);
+if (Node)
+  CommentBlock->Children.emplace_back(std::move(Node));

```
for (...)  
  if (std::unique)ptr<...> Node = genHTML(Child))
CommentBlock->...
```

would be a bitmore idiomatic LLVM code. I think you used this pattern above as 
well so you should fix it there as well.



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:325
+
+  Out.emplace_back(
+  llvm::make_unique(HTMLTag::TAG_H3, EnumType + I.Name));

same thing here I think.



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:358
+
+  Out.emplace_back(llvm::make_unique(
+  HTMLTag::TAG_P, Access + I.ReturnType.Type.Name + " " + I.Name + "(" +

ditto



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:422
+if (Parents.empty())
+  Out.emplace_back(llvm::make_unique(HTMLTag::TAG_P,
+  "Inherits from " + 
VParents));

ditto



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:425
+else if (VParents.empty())
+  Out.emplace_back(llvm::make_unique(HTMLTag::TAG_P,
+  "Inherits from " + Parents));

ditto



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:428
+else
+  Out.emplace_back(llvm::make_unique(
+  HTMLTag::TAG_P, "Inherits from " + Parents + ", " + VParents));

ditto



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:432-434
+  std::vector> Members =
+  genRecordMembersBlock(I.Members);
+  std::move(Members.begin(), Members.end(), std::back_inserter(Out));

You use this a lot here and once or twice above. You could write a template 
function that takes an `std::vector&&` and moves its contents into some 
other `std::vector&` where B : A

You can use SFINAE to ensure that B is subtype of A.





Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:472-475
+std::vector> Nodes =
+genHTML(*static_cast(I), InfoTitle);
+std::move(Nodes.begin(), Nodes.end(),
+  std::back_inserter(MainContentNode->Children));

These are all cases of what I was talking about before.



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:505
+
+  F.Children.emplace_back(
+  llvm::make_unique(HTMLTag::TAG_TITLE, InfoTitle));

ditto.



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:507
+  llvm::make_unique(HTMLTag::TAG_TITLE, InfoTitle));
+  F.Children.push_back(std::move(MainContentNode));
+

Use `emplace_back` when moving to avoid extra constructor calls.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63857/new/

https://reviews.llvm.org/D63857



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63857: [clang-doc] Add a structured HTML generator

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

As in I'm not quite ready for this to land but It looks preety solid. I think 
if you respond to those comments I'll take one last look and then give an 
actual accept


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63857/new/

https://reviews.llvm.org/D63857



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63663: [clang-doc] Add html links to references

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

The code looks good to me. I'll let Julie give the final architectural approval.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63663/new/

https://reviews.llvm.org/D63663



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

This shouldn't emit the .tbe format at all, the .tbe format is meant to be in 
near 1-1 correspondence with .so files, not with .o files. It has things like 
DT_NEEDED, and DT_SONAME related information that don't make sense for .o files 
for instance.

If we put things under an --experimental flags, minimize what we add in the 
first patch, and try and make each additional field added need based, I'm ok 
with whatever being added to Clang assuming it doesn't interfere with other 
parts of the compiler.




Comment at: llvm/include/llvm/TextAPI/ELF/ELFStub.h:58
   ELFArch Arch;
+  Optional Endian;
   std::vector NeededLibs;

Why do we need or want this? If it is needed can it be added later when needed? 
Also why is this a string?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39627: Fix vtable not receiving hidden visibility when using push(visibility)

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich updated this revision to Diff 124422.
jakehehrlich added a comment.

This test was failing on windows machines. I'm hoping this change (adding 
"_cc1" to "%clang") will resolve this because %clang_cc1 shouldn't behave any 
differently on windows.


Repository:
  rC Clang

https://reviews.llvm.org/D39627

Files:
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CGVTT.cpp
  lib/CodeGen/CGVTables.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/CodeGenModule.h
  lib/CodeGen/ItaniumCXXABI.cpp
  test/CodeGen/push-hidden-visibility-subclass.cpp

Index: test/CodeGen/push-hidden-visibility-subclass.cpp
===
--- /dev/null
+++ test/CodeGen/push-hidden-visibility-subclass.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -emit-llvm -o - %s | FileCheck %s
+
+#pragma GCC visibility push(hidden)
+
+struct Base {
+  virtual ~Base() = default;
+  virtual void* Alloc() = 0;
+};
+
+class Child : public Base {
+public:
+  Child() = default;
+  void* Alloc();
+};
+
+void test() {
+  Child x;
+}
+
+// CHECK: @_ZTV5Child = external hidden unnamed_addr constant
Index: lib/CodeGen/ItaniumCXXABI.cpp
===
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -1527,7 +1527,7 @@
 VTable->setComdat(CGM.getModule().getOrInsertComdat(VTable->getName()));
 
   // Set the right visibility.
-  CGM.setGlobalVisibility(VTable, RD);
+  CGM.setGlobalVisibility(VTable, RD, ForDefinition);
 
   // Use pointer alignment for the vtable. Otherwise we would align them based
   // on the size of the initializer which doesn't make sense as only single
@@ -1637,6 +1637,7 @@
   VTable = CGM.CreateOrReplaceCXXRuntimeVariable(
   Name, VTableType, llvm::GlobalValue::ExternalLinkage);
   VTable->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
+  CGM.setGlobalVisibility(VTable, RD, NotForDefinition);
 
   if (RD->hasAttr())
 VTable->setDLLStorageClass(llvm::GlobalValue::DLLImportStorageClass);
Index: lib/CodeGen/CodeGenModule.h
===
--- lib/CodeGen/CodeGenModule.h
+++ lib/CodeGen/CodeGenModule.h
@@ -710,7 +710,8 @@
   llvm::ConstantInt *getSize(CharUnits numChars);
 
   /// Set the visibility for the given LLVM GlobalValue.
-  void setGlobalVisibility(llvm::GlobalValue *GV, const NamedDecl *D) const;
+  void setGlobalVisibility(llvm::GlobalValue *GV, const NamedDecl *D,
+   ForDefinition_t IsForDefinition) const;
 
   /// Set the TLS mode for the given LLVM GlobalValue for the thread-local
   /// variable declaration D.
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -662,16 +662,18 @@
 }
 
 void CodeGenModule::setGlobalVisibility(llvm::GlobalValue *GV,
-const NamedDecl *D) const {
+const NamedDecl *D,
+ForDefinition_t IsForDefinition) const {
   // Internal definitions always have default visibility.
   if (GV->hasLocalLinkage()) {
 GV->setVisibility(llvm::GlobalValue::DefaultVisibility);
 return;
   }
 
   // Set visibility for definitions.
   LinkageInfo LV = D->getLinkageAndVisibility();
-  if (LV.isVisibilityExplicit() || !GV->hasAvailableExternallyLinkage())
+  if (LV.isVisibilityExplicit() ||
+  (IsForDefinition && !GV->hasAvailableExternallyLinkage()))
 GV->setVisibility(GetLLVMVisibility(LV.getVisibility()));
 }
 
@@ -1052,7 +1054,7 @@
 void CodeGenModule::SetCommonAttributes(const Decl *D,
 llvm::GlobalValue *GV) {
   if (const auto *ND = dyn_cast_or_null(D))
-setGlobalVisibility(GV, ND);
+setGlobalVisibility(GV, ND, ForDefinition);
   else
 GV->setVisibility(llvm::GlobalValue::DefaultVisibility);
 
@@ -1108,8 +1110,8 @@
   setNonAliasAttributes(D, F);
 }
 
-static void setLinkageAndVisibilityForGV(llvm::GlobalValue *GV,
- const NamedDecl *ND) {
+static void setLinkageForGV(llvm::GlobalValue *GV,
+const NamedDecl *ND) {
   // Set linkage and visibility in case we never see a definition.
   LinkageInfo LV = ND->getLinkageAndVisibility();
   if (!isExternallyVisible(LV.getLinkage())) {
@@ -1125,10 +1127,6 @@
   // separate linkage types for this.
   GV->setLinkage(llvm::GlobalValue::ExternalWeakLinkage);
 }
-
-// Set visibility on a declaration only if it's explicit.
-if (LV.isVisibilityExplicit())
-  GV->setVisibility(CodeGenModule::GetLLVMVisibility(LV.getVisibility()));
   }
 }
 
@@ -1197,7 +1195,8 @@
   // Only a few attributes are set on declarations; these may later be
   // overridden by a definition.
 
-  setLinkageAndVisibilityForGV(F, FD);
+  setLinkageForGV(F, FD);
+  setGlobalVisibility(F, FD, NotF

[PATCH] D39627: Fix vtable not receiving hidden visibility when using push(visibility)

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich updated this revision to Diff 124646.
jakehehrlich added a comment.

When I changed the test to add "_cc1" I got rid of the temporary file. Well for 
some reason on windows we're still getting different output so we need the 
temporary file to debug things.


Repository:
  rL LLVM

https://reviews.llvm.org/D39627

Files:
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CGVTT.cpp
  lib/CodeGen/CGVTables.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/CodeGenModule.h
  lib/CodeGen/ItaniumCXXABI.cpp
  test/CodeGen/push-hidden-visibility-subclass.cpp

Index: test/CodeGen/push-hidden-visibility-subclass.cpp
===
--- /dev/null
+++ test/CodeGen/push-hidden-visibility-subclass.cpp
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -emit-llvm %s -o %t
+// RUN: FileCheck --input-file=%t %s
+
+#pragma GCC visibility push(hidden)
+
+struct Base {
+  virtual ~Base() = default;
+  virtual void* Alloc() = 0;
+};
+
+class Child : public Base {
+public:
+  Child() = default;
+  void* Alloc();
+};
+
+void test() {
+  Child x;
+}
+
+// CHECK: @_ZTV5Child = external hidden unnamed_addr constant
Index: lib/CodeGen/ItaniumCXXABI.cpp
===
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -1527,7 +1527,7 @@
 VTable->setComdat(CGM.getModule().getOrInsertComdat(VTable->getName()));
 
   // Set the right visibility.
-  CGM.setGlobalVisibility(VTable, RD);
+  CGM.setGlobalVisibility(VTable, RD, ForDefinition);
 
   // Use pointer alignment for the vtable. Otherwise we would align them based
   // on the size of the initializer which doesn't make sense as only single
@@ -1637,6 +1637,7 @@
   VTable = CGM.CreateOrReplaceCXXRuntimeVariable(
   Name, VTableType, llvm::GlobalValue::ExternalLinkage);
   VTable->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
+  CGM.setGlobalVisibility(VTable, RD, NotForDefinition);
 
   if (RD->hasAttr())
 VTable->setDLLStorageClass(llvm::GlobalValue::DLLImportStorageClass);
Index: lib/CodeGen/CodeGenModule.h
===
--- lib/CodeGen/CodeGenModule.h
+++ lib/CodeGen/CodeGenModule.h
@@ -710,7 +710,8 @@
   llvm::ConstantInt *getSize(CharUnits numChars);
 
   /// Set the visibility for the given LLVM GlobalValue.
-  void setGlobalVisibility(llvm::GlobalValue *GV, const NamedDecl *D) const;
+  void setGlobalVisibility(llvm::GlobalValue *GV, const NamedDecl *D,
+   ForDefinition_t IsForDefinition) const;
 
   /// Set the TLS mode for the given LLVM GlobalValue for the thread-local
   /// variable declaration D.
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -669,16 +669,18 @@
 }
 
 void CodeGenModule::setGlobalVisibility(llvm::GlobalValue *GV,
-const NamedDecl *D) const {
+const NamedDecl *D,
+ForDefinition_t IsForDefinition) const {
   // Internal definitions always have default visibility.
   if (GV->hasLocalLinkage()) {
 GV->setVisibility(llvm::GlobalValue::DefaultVisibility);
 return;
   }
 
   // Set visibility for definitions.
   LinkageInfo LV = D->getLinkageAndVisibility();
-  if (LV.isVisibilityExplicit() || !GV->hasAvailableExternallyLinkage())
+  if (LV.isVisibilityExplicit() ||
+  (IsForDefinition && !GV->hasAvailableExternallyLinkage()))
 GV->setVisibility(GetLLVMVisibility(LV.getVisibility()));
 }
 
@@ -1059,7 +1061,7 @@
 void CodeGenModule::SetCommonAttributes(const Decl *D,
 llvm::GlobalValue *GV) {
   if (const auto *ND = dyn_cast_or_null(D))
-setGlobalVisibility(GV, ND);
+setGlobalVisibility(GV, ND, ForDefinition);
   else
 GV->setVisibility(llvm::GlobalValue::DefaultVisibility);
 
@@ -1115,8 +1117,8 @@
   setNonAliasAttributes(D, F);
 }
 
-static void setLinkageAndVisibilityForGV(llvm::GlobalValue *GV,
- const NamedDecl *ND) {
+static void setLinkageForGV(llvm::GlobalValue *GV,
+const NamedDecl *ND) {
   // Set linkage and visibility in case we never see a definition.
   LinkageInfo LV = ND->getLinkageAndVisibility();
   if (!isExternallyVisible(LV.getLinkage())) {
@@ -1132,10 +1134,6 @@
   // separate linkage types for this.
   GV->setLinkage(llvm::GlobalValue::ExternalWeakLinkage);
 }
-
-// Set visibility on a declaration only if it's explicit.
-if (LV.isVisibilityExplicit())
-  GV->setVisibility(CodeGenModule::GetLLVMVisibility(LV.getVisibility()));
   }
 }
 
@@ -1204,7 +1202,8 @@
   // Only a few attributes are set on declarations; these may later be
   // overridden by a definition.
 
-  setLinkageAndVisibilityForGV(F, FD);
+  setLinkageForGV(F, FD);

[PATCH] D66712: [Driver] Enable ShadowCallStack, not SafeStack, by default on AArch64 Fuchsia

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich accepted this revision.
jakehehrlich added a comment.
This revision is now accepted and ready to land.

Landing aside, this looks good to me


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66712/new/

https://reviews.llvm.org/D66712



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46791: Make -gsplit-dwarf generally available

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

This is obsolete I think. Clang doesn't use llvm-objcopy for this anymore if I 
understand correct. @pcc should be able to confirm.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D46791/new/

https://reviews.llvm.org/D46791



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36194: [CMake] Include llvm-objcopy tool in Fuchsia toolchain

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich accepted this revision.
jakehehrlich added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rL LLVM

https://reviews.llvm.org/D36194



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44912: [clang-doc] Removing -Wunused-variable warning

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich accepted this revision.
jakehehrlich added a comment.
This revision is now accepted and ready to land.

LGTM


https://reviews.llvm.org/D44912



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39821: Add CLANG_DEFAULT_OBJCOPY to allow Clang to use llvm-objcopy for dwarf fission

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich created this revision.
Herald added subscribers: aprantl, mgorny.

llvm-objcopy is getting to where it can be used in non-trivial ways (such as 
for dwarf fission in clang). It now supports dwarf fission but this feature 
hasn't been thoroughly tested yet. This change allows people to optionally 
build clang to use llvm-objcopy rather than GNU objcopy. By default GNU objcopy 
is still used so nothing should change.


https://reviews.llvm.org/D39821

Files:
  CMakeLists.txt
  include/clang/Config/config.h.cmake
  lib/Driver/ToolChains/CommonArgs.cpp


Index: lib/Driver/ToolChains/CommonArgs.cpp
===
--- lib/Driver/ToolChains/CommonArgs.cpp
+++ lib/Driver/ToolChains/CommonArgs.cpp
@@ -725,7 +725,8 @@
   ExtractArgs.push_back(Output.getFilename());
   ExtractArgs.push_back(OutFile);
 
-  const char *Exec = Args.MakeArgString(TC.GetProgramPath("objcopy"));
+  const char *Exec =
+  Args.MakeArgString(TC.GetProgramPath(CLANG_DEFAULT_OBJCOPY));
   InputInfo II(types::TY_Object, Output.getFilename(), Output.getFilename());
 
   // First extract the dwo sections.
Index: include/clang/Config/config.h.cmake
===
--- include/clang/Config/config.h.cmake
+++ include/clang/Config/config.h.cmake
@@ -17,6 +17,9 @@
 /* Default runtime library to use. */
 #define CLANG_DEFAULT_RTLIB "${CLANG_DEFAULT_RTLIB}"
 
+/* Default objcopy to use */
+#define CLANG_DEFAULT_OBJCOPY "${CLANG_DEFAULT_OBJCOPY}"
+
 /* Default OpenMP runtime used by -fopenmp. */
 #define CLANG_DEFAULT_OPENMP_RUNTIME "${CLANG_DEFAULT_OPENMP_RUNTIME}"
 
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -232,6 +232,9 @@
 "Default runtime library to use (\"libgcc\" or \"compiler-rt\", empty for 
platform default)" FORCE)
 endif()
 
+set(CLANG_DEFAULT_OBJCOPY "objcopy" CACHE STRING
+  "Default objcopy runtime to use.")
+
 set(CLANG_DEFAULT_OPENMP_RUNTIME "libomp" CACHE STRING
   "Default OpenMP runtime used by -fopenmp.")
 


Index: lib/Driver/ToolChains/CommonArgs.cpp
===
--- lib/Driver/ToolChains/CommonArgs.cpp
+++ lib/Driver/ToolChains/CommonArgs.cpp
@@ -725,7 +725,8 @@
   ExtractArgs.push_back(Output.getFilename());
   ExtractArgs.push_back(OutFile);
 
-  const char *Exec = Args.MakeArgString(TC.GetProgramPath("objcopy"));
+  const char *Exec =
+  Args.MakeArgString(TC.GetProgramPath(CLANG_DEFAULT_OBJCOPY));
   InputInfo II(types::TY_Object, Output.getFilename(), Output.getFilename());
 
   // First extract the dwo sections.
Index: include/clang/Config/config.h.cmake
===
--- include/clang/Config/config.h.cmake
+++ include/clang/Config/config.h.cmake
@@ -17,6 +17,9 @@
 /* Default runtime library to use. */
 #define CLANG_DEFAULT_RTLIB "${CLANG_DEFAULT_RTLIB}"
 
+/* Default objcopy to use */
+#define CLANG_DEFAULT_OBJCOPY "${CLANG_DEFAULT_OBJCOPY}"
+
 /* Default OpenMP runtime used by -fopenmp. */
 #define CLANG_DEFAULT_OPENMP_RUNTIME "${CLANG_DEFAULT_OPENMP_RUNTIME}"
 
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -232,6 +232,9 @@
 "Default runtime library to use (\"libgcc\" or \"compiler-rt\", empty for platform default)" FORCE)
 endif()
 
+set(CLANG_DEFAULT_OBJCOPY "objcopy" CACHE STRING
+  "Default objcopy runtime to use.")
+
 set(CLANG_DEFAULT_OPENMP_RUNTIME "libomp" CACHE STRING
   "Default OpenMP runtime used by -fopenmp.")
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39821: Add CLANG_DEFAULT_OBJCOPY to allow Clang to use llvm-objcopy for dwarf fission

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich updated this revision to Diff 122382.
jakehehrlich added a comment.

fixed typo


Repository:
  rL LLVM

https://reviews.llvm.org/D39821

Files:
  CMakeLists.txt
  include/clang/Config/config.h.cmake
  lib/Driver/ToolChains/CommonArgs.cpp


Index: lib/Driver/ToolChains/CommonArgs.cpp
===
--- lib/Driver/ToolChains/CommonArgs.cpp
+++ lib/Driver/ToolChains/CommonArgs.cpp
@@ -725,7 +725,8 @@
   ExtractArgs.push_back(Output.getFilename());
   ExtractArgs.push_back(OutFile);
 
-  const char *Exec = Args.MakeArgString(TC.GetProgramPath("objcopy"));
+  const char *Exec =
+  Args.MakeArgString(TC.GetProgramPath(CLANG_DEFAULT_OBJCOPY));
   InputInfo II(types::TY_Object, Output.getFilename(), Output.getFilename());
 
   // First extract the dwo sections.
Index: include/clang/Config/config.h.cmake
===
--- include/clang/Config/config.h.cmake
+++ include/clang/Config/config.h.cmake
@@ -17,6 +17,9 @@
 /* Default runtime library to use. */
 #define CLANG_DEFAULT_RTLIB "${CLANG_DEFAULT_RTLIB}"
 
+/* Default objcopy to use */
+#define CLANG_DEFAULT_OBJCOPY "${CLANG_DEFAULT_OBJCOPY}"
+
 /* Default OpenMP runtime used by -fopenmp. */
 #define CLANG_DEFAULT_OPENMP_RUNTIME "${CLANG_DEFAULT_OPENMP_RUNTIME}"
 
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -232,6 +232,9 @@
 "Default runtime library to use (\"libgcc\" or \"compiler-rt\", empty for 
platform default)" FORCE)
 endif()
 
+set(CLANG_DEFAULT_OBJCOPY "objcopy" CACHE STRING
+  "Default objcopy executable to use.")
+
 set(CLANG_DEFAULT_OPENMP_RUNTIME "libomp" CACHE STRING
   "Default OpenMP runtime used by -fopenmp.")
 


Index: lib/Driver/ToolChains/CommonArgs.cpp
===
--- lib/Driver/ToolChains/CommonArgs.cpp
+++ lib/Driver/ToolChains/CommonArgs.cpp
@@ -725,7 +725,8 @@
   ExtractArgs.push_back(Output.getFilename());
   ExtractArgs.push_back(OutFile);
 
-  const char *Exec = Args.MakeArgString(TC.GetProgramPath("objcopy"));
+  const char *Exec =
+  Args.MakeArgString(TC.GetProgramPath(CLANG_DEFAULT_OBJCOPY));
   InputInfo II(types::TY_Object, Output.getFilename(), Output.getFilename());
 
   // First extract the dwo sections.
Index: include/clang/Config/config.h.cmake
===
--- include/clang/Config/config.h.cmake
+++ include/clang/Config/config.h.cmake
@@ -17,6 +17,9 @@
 /* Default runtime library to use. */
 #define CLANG_DEFAULT_RTLIB "${CLANG_DEFAULT_RTLIB}"
 
+/* Default objcopy to use */
+#define CLANG_DEFAULT_OBJCOPY "${CLANG_DEFAULT_OBJCOPY}"
+
 /* Default OpenMP runtime used by -fopenmp. */
 #define CLANG_DEFAULT_OPENMP_RUNTIME "${CLANG_DEFAULT_OPENMP_RUNTIME}"
 
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -232,6 +232,9 @@
 "Default runtime library to use (\"libgcc\" or \"compiler-rt\", empty for platform default)" FORCE)
 endif()
 
+set(CLANG_DEFAULT_OBJCOPY "objcopy" CACHE STRING
+  "Default objcopy executable to use.")
+
 set(CLANG_DEFAULT_OPENMP_RUNTIME "libomp" CACHE STRING
   "Default OpenMP runtime used by -fopenmp.")
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39821: Add CLANG_DEFAULT_OBJCOPY to allow Clang to use llvm-objcopy for dwarf fission

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

In https://reviews.llvm.org/D39821#921703, @Hahnfeld wrote:

> I think the current tests will only pass iff the executable name ends with 
> `objcopy` - can we make this more reliable?


If I read the tests (split-debug.h, split-debug.c, and split-debug.s) correctly 
they check for the existence of the word "objcopy" somewhere in the executables 
name. I'm not sure how to make it anymore flexible than that. The right thing 
to do would be to somehow get the string from CMake and use that in this test. 
I'm not sure of llvm-lit supports any kind of find and replace like that 
however.


Repository:
  rL LLVM

https://reviews.llvm.org/D39821



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39821: Add CLANG_DEFAULT_OBJCOPY to allow Clang to use llvm-objcopy for dwarf fission

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

> Another option would be to restrict `CLANG_DEFAULT_OBJCOPY` to contain 
> `objcopy` or even to only allow `objcopy` or `llvm-objcopy`. I don't know if 
> that's sensible though...

I think there are some other strip/objcopy like implementations out there but I 
think objcopy and llvm-objcopy are the only ones that support --extract-dwo and 
--strip-dwo right now but more could come. I think objcopy and llvm-objcopy are 
the only ones that have "objcopy" in the name so the current test kind of 
enforces that one of them must be used. Also the user might want to pull 
objcopy or llvm-objcopy from a specific place on their system (not just from 
PATH) so however we restrict this, it will have to be much more general than 
just allowing "objcopy" or "llvm-objcopy". That said restricting the set of 
program names seems reasonable to me since you should in theory be able to add 
your program to the CMakeList.txt check on CLANG_DEFAULT_OBJCOPY if you want 
clang to support it. I don't think any other implementation of objcopy is 
likely to exist anytime soon though so it seems kind of moot to me.


Repository:
  rL LLVM

https://reviews.llvm.org/D39821



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39821: Add CLANG_DEFAULT_OBJCOPY to allow Clang to use llvm-objcopy for dwarf fission

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich closed this revision.
jakehehrlich added a comment.

This has already landed, I'm not sure why it wasn't automatically closed.


Repository:
  rL LLVM

https://reviews.llvm.org/D39821



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39627: Fix vtable not receiving hidden visibility when using push(visibility)

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich updated this revision to Diff 123399.
jakehehrlich added a comment.

1. Added a ForDefinition parameter to setGlobalVisibility and merged visibility 
setting behaviors for setGlobalVisibility and setLinkageAndVisibilityForGV
2. Renamed setLinkageAndVisibilityForGV to setLinkageForGV
3. refactored calls to these two functions to match their new form.


Repository:
  rL LLVM

https://reviews.llvm.org/D39627

Files:
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CGVTT.cpp
  lib/CodeGen/CGVTables.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/CodeGenModule.h
  lib/CodeGen/ItaniumCXXABI.cpp
  test/CodeGen/push-hidden-visibility-subclass.cpp

Index: test/CodeGen/push-hidden-visibility-subclass.cpp
===
--- /dev/null
+++ test/CodeGen/push-hidden-visibility-subclass.cpp
@@ -0,0 +1,21 @@
+// RUN: %clang -S -emit-llvm -o %t %s
+// RUN: FileCheck --input-file=%t %s
+
+#pragma GCC visibility push(hidden)
+
+struct Base {
+  virtual ~Base() = default;
+  virtual void* Alloc() = 0;
+};
+
+class Child : public Base {
+public:
+  Child() = default;
+  void* Alloc();
+};
+
+void test() {
+  Child x;
+}
+
+// CHECK: @_ZTV5Child = external hidden unnamed_addr constant
Index: lib/CodeGen/ItaniumCXXABI.cpp
===
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -1527,7 +1527,7 @@
 VTable->setComdat(CGM.getModule().getOrInsertComdat(VTable->getName()));
 
   // Set the right visibility.
-  CGM.setGlobalVisibility(VTable, RD);
+  CGM.setGlobalVisibility(VTable, RD, ForDefinition);
 
   // Use pointer alignment for the vtable. Otherwise we would align them based
   // on the size of the initializer which doesn't make sense as only single
@@ -1637,6 +1637,7 @@
   VTable = CGM.CreateOrReplaceCXXRuntimeVariable(
   Name, VTableType, llvm::GlobalValue::ExternalLinkage);
   VTable->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
+  CGM.setGlobalVisibility(VTable, RD, NotForDefinition);
 
   if (RD->hasAttr())
 VTable->setDLLStorageClass(llvm::GlobalValue::DLLImportStorageClass);
Index: lib/CodeGen/CodeGenModule.h
===
--- lib/CodeGen/CodeGenModule.h
+++ lib/CodeGen/CodeGenModule.h
@@ -710,7 +710,8 @@
   llvm::ConstantInt *getSize(CharUnits numChars);
 
   /// Set the visibility for the given LLVM GlobalValue.
-  void setGlobalVisibility(llvm::GlobalValue *GV, const NamedDecl *D) const;
+  void setGlobalVisibility(llvm::GlobalValue *GV, const NamedDecl *D,
+   ForDefinition_t IsForDefinition) const;
 
   /// Set the TLS mode for the given LLVM GlobalValue for the thread-local
   /// variable declaration D.
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -662,16 +662,18 @@
 }
 
 void CodeGenModule::setGlobalVisibility(llvm::GlobalValue *GV,
-const NamedDecl *D) const {
+const NamedDecl *D,
+ForDefinition_t IsForDefinition) const {
   // Internal definitions always have default visibility.
   if (GV->hasLocalLinkage()) {
 GV->setVisibility(llvm::GlobalValue::DefaultVisibility);
 return;
   }
 
   // Set visibility for definitions.
   LinkageInfo LV = D->getLinkageAndVisibility();
-  if (LV.isVisibilityExplicit() || !GV->hasAvailableExternallyLinkage())
+  if (LV.isVisibilityExplicit() ||
+  (IsForDefinition && !GV->hasAvailableExternallyLinkage()))
 GV->setVisibility(GetLLVMVisibility(LV.getVisibility()));
 }
 
@@ -1052,7 +1054,7 @@
 void CodeGenModule::SetCommonAttributes(const Decl *D,
 llvm::GlobalValue *GV) {
   if (const auto *ND = dyn_cast_or_null(D))
-setGlobalVisibility(GV, ND);
+setGlobalVisibility(GV, ND, ForDefinition);
   else
 GV->setVisibility(llvm::GlobalValue::DefaultVisibility);
 
@@ -1108,8 +1110,8 @@
   setNonAliasAttributes(D, F);
 }
 
-static void setLinkageAndVisibilityForGV(llvm::GlobalValue *GV,
- const NamedDecl *ND) {
+static void setLinkageForGV(llvm::GlobalValue *GV,
+const NamedDecl *ND) {
   // Set linkage and visibility in case we never see a definition.
   LinkageInfo LV = ND->getLinkageAndVisibility();
   if (!isExternallyVisible(LV.getLinkage())) {
@@ -1125,10 +1127,6 @@
   // separate linkage types for this.
   GV->setLinkage(llvm::GlobalValue::ExternalWeakLinkage);
 }
-
-// Set visibility on a declaration only if it's explicit.
-if (LV.isVisibilityExplicit())
-  GV->setVisibility(CodeGenModule::GetLLVMVisibility(LV.getVisibility()));
   }
 }
 
@@ -1197,7 +1195,8 @@
   // Only a few attributes are set on declarations; these may later be
   // over

[PATCH] D42275: [Fuchsia] Enable Fuzzer as a supported sanitizer on Fuchsia

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich accepted this revision.
jakehehrlich added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rC Clang

https://reviews.llvm.org/D42275



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41102: Setup clang-doc frontend framework

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

If its possible to split VisitEnumDecl, and VisitRecordDecl into separate 
methods and the same is possible for VisitFunctionDecl and VisitCXXMethodDecl 
then I think all of your methods will look like the following 
VisitNamespaceDecl. That being the case you might want to factor this out 
somehow (which I think also would resolve my comment about isUnparsed being 
used the same way a lot).

for instance you might have a function like

  template 
  bool ClangDocVisitor::visitDecl(const T *D) {
if (!isUnparsed(D->getLocation()))
  return true;
Reporter.createInfo(D, getComment(D), getLine(D), getFile(D)); // Use 
explicit template specialization to make "createInfo" uniform
return true;
  }

and then define a macro like the following

  #define DEFINE_VISIT_METHOD(TYPE) \
  bool ClangDocVisitor::Visit##TYPE(const TYPE *D) { return visitDecl(D); }




Comment at: tools/clang-doc/ClangDoc.cpp:22
+
+bool ClangDocVisitor::VisitTagDecl(const TagDecl *D) {
+  if (!isUnparsed(D->getLocation()))

Is it possible to use VisitEnumDecl and VisitRecordDecl separately here?



Comment at: tools/clang-doc/ClangDoc.cpp:35
+
+  // Error?
+  return true;

I think you should use llvm_unrechable here



Comment at: tools/clang-doc/ClangDoc.cpp:40
+bool ClangDocVisitor::VisitNamespaceDecl(const NamespaceDecl *D) {
+  if (!isUnparsed(D->getLocation()))
+return true;

It looks like you're using this pattern a lot. It might be worth factoring this 
out somehow.



Comment at: tools/clang-doc/ClangDoc.cpp:46
+
+bool ClangDocVisitor::VisitFunctionDecl(const FunctionDecl *D) {
+  if (!isUnparsed(D->getLocation()))

Can you separate this into VisitFunctionDecl and VisitCXXMethodDecl?



Comment at: tools/clang-doc/ClangDoc.cpp:60
+
+comments::FullComment *ClangDocVisitor::getComment(const Decl *D) {
+  RawComment *Comment = Context->getRawCommentForDeclNoCache(D);

Can this be a const method?



Comment at: tools/clang-doc/ClangDoc.cpp:76
+
+std::string ClangDocVisitor::getFile(const Decl *D) const {
+  PresumedLoc PLoc = Manager.getPresumedLoc(D->getLocStart());

I think this method should return a StringRef instead of an std::string because 
the const char* returned by getFilename should live at least as long as the 
source manager.



Comment at: tools/clang-doc/ClangDoc.cpp:85
+  continue;
+std::shared_ptr CI = std::make_shared();
+Reporter.parseFullComment(Comment->parse(*Context, nullptr, nullptr), CI);

So haven't looked enough at the reporter code yet but it seems to me this 
should a unique pointer. You seem to already be aware of that based on a TODO I 
saw in the reporter code though. Is it possible that "parseFullComent" should 
just take a plain old pointer instead of a unique_ptr or shared_ptr? 



Comment at: tools/clang-doc/ClangDoc.cpp:92
+
+bool ClangDocVisitor::isUnparsed(SourceLocation Loc) const {
+  if (!Loc.isValid())

Can you add a comment documenting what this function does?



Comment at: tools/clang-doc/ClangDoc.cpp:108
+  if (const auto *Ctor = dyn_cast(D))
+MC->mangleCXXCtor(Ctor, CXXCtorType::Ctor_Complete, MangledName);
+  else if (const auto *Dtor = dyn_cast(D))

I think it's kind of annoying that this can't be a const method because of 
these mangle calls. I don't really understand why MangleContext works the way 
that it does but it could be that this is a situation where the "mutable" 
keyword should be used on MC to allow what should be a const method to actully 
be const. That might be something to look into.



Comment at: tools/clang-doc/ClangDoc.cpp:113
+MC->mangleName(D, MangledName);
+  return MangledName.str();
+}

I think you want to return S here so that the move constructor is used instead. 
str() returns a reference to S which will cause the copy constructor to be 
called. I *think* most std::string implementations have a copy on write 
optimization but it's strictly more ideal to use the move constructor.



Comment at: tools/clang-doc/ClangDoc.cpp:123
+ClangDocAction::CreateASTConsumer(CompilerInstance &C, StringRef InFile) {
+  return make_unique(&C.getASTContext(), Reporter);
+}

Pro Tip: Always explicitly refer to this as "llvm::make_unique" because you'll 
have to revert this change if you don't.

Some of the build bots have C++14 headers instead of C++11 headers. This means 
that llvm::make_unique and std::make_unique will both be defined. This means 
that using "make_unique" will cause an error even though only llvm::make_unique 
can be referred to unqualified. So even if you're inside of the llvm namespace 
you should explicitly refer to "llvm::make_unique" and n

[PATCH] D41102: Setup clang-doc frontend framework

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added inline comments.



Comment at: tools/clang-doc/ClangDocReporter.cpp:53-54
+  if (Pair == Docs.Namespaces.end()) {
+std::unique_ptr I = make_unique();
+Docs.Namespaces[Name] = std::move(I);
+populateBasicInfo(*Docs.Namespaces[Name], Name, D->getNameAsString(),

There's no need for I here, also use llvm::make_unique



Comment at: tools/clang-doc/ClangDocReporter.h:133
+
+  void createNamespaceInfo(const NamespaceDecl *D, const FullComment *C,
+   int LineNumber, StringRef File);

I think you should use explicit template specialization to make these 
"createFooInfo" methods uniform. This will enable other code that calls these 
methods to be written in a more uniform fashion.

so define something like

```
template
void createInfo(const T *D, const FullComment *C, ...);
```
 
and then define various specializations of that member function instead of 
creating a new method for each createFooInfo method.


https://reviews.llvm.org/D41102



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41102: Setup clang-doc frontend framework

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added inline comments.



Comment at: tools/clang-doc/ClangDocReporter.cpp:55
+Docs.Namespaces[Name] = std::move(I);
+populateBasicInfo(*Docs.Namespaces[Name], Name, D->getNameAsString(),
+  getParentNamespace(D));

If you make a "populateNamespaceInfo" method that just calls populateBasicInfo 
but checks to see that the namespace hasn't already been added you can move 
this outside of this if statement which will make it more uniform with the 
other invocations. Also if you then specialize a general form of 
"populateInfo",  include a specialization for NamespaceInfo, and do a few more 
things in other methods I think most of these methods become identical (the 
Function stuff is still different).



Comment at: tools/clang-doc/ClangDocReporter.cpp:70
+  if (Pair == Docs.Types.end()) {
+std::unique_ptr I = make_unique();
+Docs.Types[Name] = std::move(I);

ditto



Comment at: tools/clang-doc/ClangDocReporter.cpp:74
+
+  if (D->isThisDeclarationADefinition())
+populateTypeInfo(*Docs.Types[Name], D, Name, File);

Is this a check that populateTypeInfo could do instead? Or do we sometimes want 
to call populateTypeInfo on non-definitions?



Comment at: tools/clang-doc/ClangDocReporter.cpp:87
+  if (Pair == Docs.Enums.end()) {
+std::unique_ptr I = make_unique();
+Docs.Enums[Name] = std::move(I);

ditto



Comment at: tools/clang-doc/ClangDocReporter.cpp:91
+
+  if (D->isThisDeclarationADefinition())
+populateEnumInfo(*Docs.Enums[Name], D, Name, File);

Same comment here as I had on createTypeInfo/populateTypeInfo



Comment at: tools/clang-doc/ClangDocReporter.cpp:118
+
+  if (D->isThisDeclarationADefinition())
+populateFunctionInfo(*Docs.Namespaces[Namespace]->Functions[MangledName], 
D,

Is this something that can go inside populateFunctionInfo?



Comment at: tools/clang-doc/ClangDocReporter.h:166
+ StringRef Namespace);
+  void populateTypeInfo(TypeInfo &I, const RecordDecl *D, StringRef Name,
+StringRef File);

sans the BasicInfo one I think you should use the same specialization trick 
here. After you do that the main difference between createInfo methods will be 
what collection they add too. That suggests to me that the collection the info 
is added to should be made a parameter to a method that does all the actual 
work.


https://reviews.llvm.org/D41102



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41102: Setup clang-doc frontend framework

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added inline comments.



Comment at: tools/clang-doc/ClangDoc.h:33
+
+class ClangDocVisitor : public RecursiveASTVisitor {
+public:

sammccall wrote:
> This API makes essentially everything public. Is that the intent?
> 
> It seems like `ClangDocVisitor` is a detail, and the operation you want to 
> expose is "extract doc from this AST into this reporter" or maybe "create an 
> AST consumer that feeds this reporter".
> 
> It would be useful to have an API to extract documentation from individual 
> AST nodes (e.g. a Decl). But I'd be nervous about trying to use the classes 
> exposed here to do that. If it's efficiently possible, it'd be nice to expose 
> a function.
> (one use case for this is clangd)
Correct me if I'm wrong but I believe that everything needs to be public in 
this case because the base class needs to be able to call them. So the visit 
methods all need to be public.



Comment at: tools/clang-doc/ClangDoc.h:39
+
+  bool VisitTagDecl(const TagDecl *D);
+  bool VisitNamespaceDecl(const NamespaceDecl *D);

sammccall wrote:
> `override` where applicable
These methods are not virtual methods. It's technically legal to use the 
override keyword if a subclass shadows a non-virtual method but I don't think 
its what we want to do here.



Comment at: tools/clang-doc/ClangDocReporter.cpp:43
+  F->Filename = Filename;
+  Docs.Files.insert(std::make_pair(Filename, std::move(F)));
+}

instead of inserting a pair can we just use '[]' syntax?



Comment at: tools/clang-doc/ClangDocReporter.cpp:114
+  if (Pair == Docs.Namespaces[Namespace]->Functions.end()) {
+std::unique_ptr I = make_unique();
+Docs.Namespaces[Namespace]->Functions[MangledName] = std::move(I);

no need for I, and use llvm::make_unique



Comment at: tools/clang-doc/ClangDocReporter.cpp:127
+
+void ClangDocReporter::createMethodInfo(const CXXMethodDecl *D,
+const FullComment *C, int LineNumber,

As I'm looking though these methods more I'm thinking you might want to break 
each of these createInfo methods up into smaller parts. For instance the 
addLocation/addComment part is the same in everyone of these, they all extract 
some name from a decl, they all use that string to get an iterator to the 
needed item. They all check to see if that iterator is the end and then add the 
item to the container etc... There's a lot more opportunity for deduplication 
if  break these things up some.



Comment at: tools/clang-doc/ClangDocReporter.cpp:142
+  if (Pair == Docs.Types[ParentName]->Functions.end()) {
+std::unique_ptr I = make_unique();
+Docs.Types[ParentName]->Functions[MangledName] = std::move(I);

ditto again



Comment at: tools/clang-doc/ClangDocReporter.cpp:155
+
+void ClangDocReporter::parseFullComment(const FullComment *C,
+std::shared_ptr &CI) {

Do we need this method?



Comment at: tools/clang-doc/ClangDocReporter.cpp:178
+NamedType T{Attr.Name, Attr.Value, clang::AccessSpecifier::AS_none};
+CurrentCI->Attrs.push_back(T);
+  }

Can we use emplace_back here instead of copying a NamedType?



Comment at: tools/clang-doc/ClangDocReporter.cpp:303
+  CI->Kind = C->getCommentKindName();
+  ConstCommentVisitor::visit(C);
+  for (comments::Comment *Child :

So it looks like the reason you need CurrentCI is because the visit methods 
need it and you need different CI's to be used at different visit calls but the 
visit methods can't take any more parameters. I think you should put the visit 
methods in another class that takes a pointer to a CommentInfo as an argument 
to the constructor. I *think* that should clean up this code smell and help 
mitigate the use of shared_ptr everywhere.



Comment at: tools/clang-doc/ClangDocReporter.cpp:355
+  yaml::Output YOut(OS);
+  for (auto &I : Map)
+YOut << *(I.second);

can these be const auto&?



Comment at: tools/clang-doc/ClangDocReporter.cpp:363
+  yaml::Output YOut(OS);
+  for (auto &I : Map) {
+YOut << *(I.second);

ditto on these



Comment at: tools/clang-doc/ClangDocReporter.cpp:372
+  if (RootDir.empty()) {
+printMap>(outs(), Docs.Files);
+printMapPlusFunctions>(outs(),

Do we need to explicitly pass these types? I think template argument deduction 
should fill this in for us.



Comment at: tools/clang-doc/ClangDocReporter.cpp:383-390
+  sys::path::native(RootDir, FilePath);
+  sys::path::append(FilePath, "files.yaml");
+  raw_fd_ostream FileOS(FilePath, OutErrorInfo, sys::fs::F_None);
+  if (OutErrorInfo != OK) {
+errs() << "Error opening documentation f