In any case I have to do a large run internally because we still use -fcommon :)

Sorry for the churn. On the bright side if this change impacts any internal projects you can use it to help convince them to stop using -fcommon :).

Delete unneeded 'Function' declaration

Please include a brief message why the change is reverted in the future

In D88199#2292193, @jhenderson wrote:

Mostly looks okay. I think there is no testing for the assembly parser being able to parse the new section type?

I'm also keen to hear what others think (e.g. @MaskRay).

Checked lld/test/ELF updates, GNU ar silently allows invalid 'symbolic' files. I think being more rigid in llvm-ar is fine.

Hi @aprantl, this breaks test/Bitcode/DISubprogram-v4.ll

In D65880#2294871, @protze.joachim wrote:

I still see some clang-specific and system link directories listed in the linker line before the directories from LIBRARY_PATH:

$ LIBRARY_PATH=test1 /usr/local/clang/bin/clang -Ltest2 -v hello.c
 "/usr/bin/ld" .../crtbegin.o -Ltest2 -L/usr/lib/gcc/x86_64-redhat-linux/4.8.5 -L/usr/lib/gcc/x86_64-redhat-linux/4.8.5/../../../../lib64 -L/lib/../lib64 -L/usr/lib/../lib64 -L/usr/lib/gcc/x86_64-redhat-linux/4.8.5/../../.. -L/usr/local/clang/bin/../lib -L/lib -L/usr/lib -Ltest1

I think they are inserted by ToolChain.AddFilePathLibArgs in Gnu.cpp. Is this the intended ordering? My expectation would be

$ LIBRARY_PATH=test1 /usr/local/clang/bin/clang -Ltest2 -v hello.c
 "/usr/bin/ld" .../crtbegin.o -Ltest2 -Ltest1 -L/usr/lib/gcc/x86_64-redhat-linux/4.8.5 -L/usr/lib/gcc/x86_64-redhat-linux/4.8.5/../../../../lib64 -L/lib/../lib64 -L/usr/lib/../lib64 -L/usr/lib/gcc/x86_64-redhat-linux/4.8.5/../../.. -L/usr/local/clang/bin/../lib -L/lib -L/usr/lib

@hfinkel any opinion?

Thanks for review! I will wait until next week.

Fix lock

Good idea. I only got to know this after I read the source code. Many people probably have the same confusion.

LGTM.

LGTM.

[lld] Add a new known text prefix - ".text.split."

This looks good to me. Please add a test to test/ELF/text-section-prefix.s

[llvm]Add an option to emit cold clusters to a different section.

I probably have asked too much... but it'd be nice if you also add some documentation to llvm/docs/XRay*.rst That helps users :)

In D87953#2293162, @ianlevesque wrote:

Thanks @MaskRay - I tried to answer that question in https://reviews.llvm.org/D87953#2286430. At present we are deploying instrumentation to an arbitrary subset of our application using the instruction threshold. I would like to make the selection of how many and which functions more deterministic, and be able to instrument different subsets over time. The overhead we are concerned with is purely binary size as we are deploying to Android devices. We are using features from my previous XRay patches to omit the function index already, but the sheer number of sleds and size of the associated xray_instr_map are the limiting factor of how much we can instrument in any given app release. For our use case it is fine to gradually over a period of weeks work our way across the entire app group by group.

In D81865#2293158, @froydnj wrote:

In D81865#2293146, @MaskRay wrote:

In D81865#2293066, @dblaikie wrote:

In D81865#2293059, @MaskRay wrote:

@froydnj The committed version rG31a3c5fb45b78bdaa78d94ffcc9258e839002016 appears to be very different from the review. I guess next time your probably can upload the diff again if it is very diffierent

Judging by a cursory glance at Phab's view of the delta ( https://reviews.llvm.org/rG4b64ce7428b66cacfe74dbd9dbc29aff6dfb84af ) it /looks/ like it wasn't too different. Mostly picking up upstream changes that added "DEFERRABLE"? (I think Phab uses light green for "this changed, but only because of upstream changes" and dark green is the actual patch changes?)

Sorry for the noise. What I saw previously was a mere difference in the DIAG macro and the new isDeferable... Maybe Phab presented the diff between two Diffs to me. The updated view seems good.

I assumed that "add another parameter to a macro due to rebasing" was not a significant enough change to warrant reposting...but as this is the first patch I was committing myself, I probably should have been a bit more explicit in what I was committing (even re-asking for review? I'm not sure of the norms around rebasing in the LLVM project). My mistake!

In D81865#2293066, @dblaikie wrote:

In D81865#2293059, @MaskRay wrote:

@froydnj The committed version rG31a3c5fb45b78bdaa78d94ffcc9258e839002016 appears to be very different from the review. I guess next time your probably can upload the diff again if it is very diffierent

Judging by a cursory glance at Phab's view of the delta ( https://reviews.llvm.org/rG4b64ce7428b66cacfe74dbd9dbc29aff6dfb84af ) it /looks/ like it wasn't too different. Mostly picking up upstream changes that added "DEFERRABLE"? (I think Phab uses light green for "this changed, but only because of upstream changes" and dark green is the actual patch changes?)

In D87953#2293024, @kyulee wrote:

I think it looks good to me. @MaskRay Any further feedback on this?

@froydnj The committed version rG31a3c5fb45b78bdaa78d94ffcc9258e839002016 appears to be very different from the review. I guess next time your probably can upload the diff again if it is very diffierent

LGTM.

We usually use x86 for generic feature tests (sorry). I am fine with ppc but the test is probably worth a comment that some FORTRAN libraries on ppc require this behavior (i.e. if you no longer require this behavior and if we need some symbol table refactoring where this behavior gets in the way, we should have the right to revisit the decision. SHN_COMMON will get more and more obsoleted and hope we don't need this particular behavior in the future ;-) )

Hi Sri, a pattern used by others for relands is like:

This caused a -fsanitize=cfi issue in thinltoBackend. I reverted the patch temporarily and shared a detailed reproduce with Teresa

Looks great! Worth getting @sfertile's sign-off.

LGTM.

In rG144e57fc9535eb30e7a9a2b691bc15bd38b68a04#948689, @nemanjai wrote:

The commit that mentions this (https://reviews.llvm.org/rGf1746be6667) adds the cleanup. Please have a look at it and if it isn't the right way to clean up, modify the fix as needed. I had to make sure this doesn't continue to build up queues as it was causing build bot failures.

When you recommit, consider adding the header to the description. The original message started with "The wrong placement of add pass with optimizations led to -funique-internal-linkage-names being disabled." The header was missing.

API changes may cause problems to other backends. It is a good idea to test -DLLVM_TARGETS_TO_BUILD=ALL

@rahmanl The patch will look better if you can give a more formal approval :)

Hi, please use more descriptive subjects for future commits, e.g. Fix TableGen builds

In D87921#2288395, @Conanap wrote:

It looks like this commit is causing a few failures on nearly all PPC bots and a sanitizer bot; would it be possible to revert this commit for now until the issue is resolved?

LGTM.

LGTM.

If base == 0, to_integer can parse the 0x prefix.

FWIW I tested check-msan in a -DCMAKE_BUILD_TYPE=Release build on a
powerpc64le machine. All tests passed. I cannot connect the failure to
the clang patch.

In rG8fdac7cb7abbeeaed016ef9eb7a087458e41e33f#948494, @fhahn wrote:

It would be great if you could share more details about how to reproduce the failure. I tried to reproduce with a stage-2 build with -DLLVM_ENABLE_MODULES=On, but check-clang passed. Could you share the file/IR causing the crash?

In D87999#2285800, @rahmanl wrote:

LGTM.
Please just keep in mind that call-site-table is not rigorously tested in LLVM.

Looks great!

Thanks!

Looks great!