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

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

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

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

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

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 ___

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

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

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

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));

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

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 , StringRef CurrentDirectory, +

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

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

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

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

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

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

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

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/

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

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

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

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

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

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

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

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 : Functions) {

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

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

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

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

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

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{""}; +

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

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

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

jakehehrlich added a comment. LGTM Comment at: clang-tools-extra/clang-doc/Representation.h:66 + + bool operator<(const CommentInfo ) const { +auto FirstCI = std::tie(Kind, Text, Name, Direction, ParamName, CloseName, We should be explicit about what we

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

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

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

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

[PATCH] D60974: Clang IFSO driver action.

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

[PATCH] D60974: Clang IFSO driver action.

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"

[PATCH] D60974: Clang IFSO driver action.

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

[PATCH] D60974: Clang IFSO driver action.

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

[PATCH] D60974: Clang IFSO driver action.

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

[PATCH] D60974: Clang IFSO driver action.

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

[PATCH] D60974: Clang IFSO driver action.

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.

[PATCH] D60974: Clang IFSO driver action.

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)

[PATCH] D60974: Clang IFSO driver action.

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

[PATCH] D60974: Clang IFSO driver action.

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

[PATCH] D60974: Clang IFSO driver action.

jakehehrlich added a comment. In D60974#1483399 , @plotfi wrote: > In D60974#1483265 , @jakehehrlich > wrote: > > > In D60974#1483240 , @plotfi wrote: > > > > > Me and

[PATCH] D60974: Clang IFSO driver action.

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

[PATCH] D60974: Clang IFSO driver action.

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",

[PATCH] D60974: Clang IFSO driver action.

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

[PATCH] D60974: Clang IFSO driver action.

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

[PATCH] D60974: Clang IFSO driver action.

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

[PATCH] D60974: Clang IFSO driver action.

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

[PATCH] D60974: Clang IFSO driver action.

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

[PATCH] D60974: Clang IFSO driver action.

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/

[PATCH] D60974: Clang IFSO driver action.

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

[PATCH] D60974: Clang IFSO driver action.

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

[PATCH] D60974: Clang IFSO driver action.

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

[PATCH] D60974: Clang IFSO driver action.

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

[PATCH] D60974: Clang IFSO driver action.

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

[PATCH] D60974: Clang IFSO driver action.

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

[PATCH] D60974: Clang IFSO driver action.

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

[PATCH] D60974: Clang IFSO driver action.

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

[PATCH] D60974: Clang IFSO driver action.

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,

[PATCH] D60974: Clang IFSO driver action.

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

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

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

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

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")); +

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

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.

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

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

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

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

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

jakehehrlich added a comment. This is causing breaks in fuchsia, Code that looks like this uintptr_t last_unlogged = atomic_load_explicit(_tail, memory_order_acquire); do { if (last_unlogged == 0) return; } while (!atomic_compare_exchange_weak_explicit(_tail,

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

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)));

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

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.

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

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

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

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

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

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

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

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

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

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

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

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

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

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?

[PATCH] D41102: Setup clang-doc frontend framework

jakehehrlich added inline comments. Comment at: clang-doc/BitcodeWriter.cpp:407 + +void ClangDocBinaryWriter::writeBitstream(const EnumInfo , + BitstreamWriter , juliehockett wrote: > jakehehrlich wrote: > > lebedev.ri

[PATCH] D41102: Setup clang-doc frontend framework

jakehehrlich added inline comments. Comment at: clang-doc/BitcodeWriter.cpp:407 + +void ClangDocBinaryWriter::writeBitstream(const EnumInfo , + BitstreamWriter , lebedev.ri wrote: > juliehockett wrote: > > lebedev.ri

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

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

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

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

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

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:

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

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.

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

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; +

[PATCH] D41102: Setup clang-doc frontend framework

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] D41102: Setup clang-doc frontend framework

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) { +

[PATCH] D41102: Setup clang-doc frontend framework

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

[PATCH] D41102: Setup clang-doc frontend framework

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

[PATCH] D41102: Setup clang-doc frontend framework

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

[PATCH] D41102: Setup clang-doc frontend framework

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,

[PATCH] D41102: Setup clang-doc frontend framework

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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