There have been some changes in the relocation area. Is this patch unneeded now?

(back from a long vacation with little time spending on reviews.llvm.org )

Thank you for splitting! I am just back from a long vacation to check some emails and will start another long vacation soon (03/29)🤣 It will likely take some time for me to read through this change:)

In D142888#4217880, @omjavaid wrote:

Hi I was wondering if we could fast track merge of this patch into LLVM main and 16.0.0 release branch. This patch fixes LLVM 16.0.0 release build for armv7 Linux and release is waiting on this patch to be merged.

Apparently patch looks in good shape so I was wondering if any one of the reviewers can press the accept button if there are no further objections.

Adjust some terms.

In D146686#4216612, @yaxunl wrote:

In D146686#4215577, @MaskRay wrote:

This change is correct for Linux. llvm/CMakeLists.txt says:

if(CMAKE_SYSTEM_NAME MATCHES "BSD|Linux|OS390")
  set(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR_default ON)

Some rpath using OSes (notably macOS) use LLVM_ENABLE_PER_TARGET_RUNTIME_DIR_default=OFF. Is the rpath setting ever usable on macOS?
If not, the change is correct.

How about I try to add both? there is check whether the dir exist then adding it, so only the existing one will be added. then it should work in either case.

This change is correct for Linux. llvm/CMakeLists.txt says:

if(CMAKE_SYSTEM_NAME MATCHES "BSD|Linux|OS390")
  set(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR_default ON)

Thank you!

In D145646#4213547, @gulfem wrote:

We started seeing a test failure in avr-ld.c.

/b/s/w/ir/x/w/llvm-llvm-project/clang/test/Driver/avr-ld.c:48:11: error: 
// LINKP: {{".*ld.*"}} {{.*}} "--defsym=__DATA_REGION_ORIGIN__=0x800100" "-plugin" {{.*}} "-plugin-opt=mcpu=atmega328"
          ^
<stdin>:1:1: note: scanning from here
Fuchsia clang version 17.0.0 (https://llvm.googlesource.com/llvm-project 0d37efdbc599e61ce2a0418723a66d6b45aea8d7)
^
<stdin>:6:239: note: possible intended match here

https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8785950851645447937/overview

The unknown_ext_version.s case is common (for GCC uses) and will get improved.

No objection. LG once @jrtc27 is happy.

"Close" seems more appropriate to me as this isn't a bug (previously not an intended use case).

.

Sorry for my belated response as I was on a trip for quite a few days.

LG! llvm/test/CodeGen/Mips/ tests are unrelated to the driver change and the part should be committed separately.

Do you have a use case for the more flexible function signature?

for current GNU Binutils releases.

Thanks!

In D146061#4194866, @kuhar wrote:

In D146061#4194853, @nikic wrote:

In D146061#4194752, @kuhar wrote:

Sure, I agree with that. If I have map with a known type like DenseMap I'd rather call .contains(x) directly too. But this is not possible with C++17 containers std::, so I'd argue that until then is_contained(container, x) is more readable than something like container.find(x) != container.end().

I don't think we should promote usage of is_contained() for anything but the linear scan case. I don't want to be left guessing whether this is_contained() is a linear scan, or just a slightly different way to write container.find().

I can see this being prefered from the perspective of an experience llvm developer, but on the other hand I saw a lot of code that ended up using is_contained and llvm::find with map/set types. I found fixing those inefficiencies and re-explaining this to folks needlessly time consuming. I think defaulting to the more efficient implementation of containment check is the way to go to avoid those non-obvious performance issues, and the member functions we would delegate to are not surprising at all.

Is this worth a unit test?

LGTM

While --no-undefined is an alias for -z defs, be consistent in the subject and the body.

Thanks!

In D144596#4186838, @tejohnson wrote:

In D144596#4182584, @MaskRay wrote:

While we're here, let's make two more changes. First, let's use .bc instead of .o for the bitcode file extension name.

I'm not in favor of hardcoding the extension in the compiler, it should be left to the build system, but I don't believe that what you are proposing would require a specific extension anyway. Currently, the object output file extension of the compile action is not a hardcoded requirement and I don't think it should be. Additionally, when we were initially implementing ThinLTO I recall that the Bazel team specifically wanted to keep the .o extension (otherwise the handling gets a bit trickier for them as a compile action will have different extensions depending on the optimization options). But like I said it doesn't appear to me that what you are proposing would require this.

Fix -gdwarf-aranges

computeTargetTriple has several users. You probably want to check all these code paths do not need if (const Arg *A = Args.getLastArg(options::OPT_target)).
For the uses in BuildCompilation, it seems fine.

(I'm going to be out for 3 weeks in the next 4 weeks. Happy if others are happy. Don't wait on me :) )

I have an analysis of the code path: https://reviews.llvm.org/D145791#4185609
There are some if conditions, virtual function calls, SmallVector range append overhead which cannot be eliminated.

LGTM, but make sure @RKSimon is happy as well.

In D143306#4183759, @tstellar wrote:

Should we apply this patch to the release/16.x branch to create a more smooth transition since the option has been removed in main?

Thanks! LGTM from my review. It'd be good to have a review from a domain expert.

Due to this we started seeing assembler errors with certain .c and .cpp files -
"Error: file number 1 already allocated"

In D118493#4182583, @JonChesterfield wrote:

Duplicating a comment from the commit thread so it's easier for me to find later.

You've applied this to the release branches going back as far as 14. It's a user facing breaking change. As in people who have a working openmp toolchain and update to the point release which looks like a semantic versioning thing indicating minor bugfixes will experience immediate cessation of function. I consider that very poor user experience and do not agree with the scope of breakage.

In D144596#4178243, @tejohnson wrote:

In D144596#4176555, @MaskRay wrote:

I find the summary difficult to follow and I believe users will be more confusing than me (as a ThinLTO contributor).

I'll take a stab at suggesting a clearer summary in a little bit.

(I'll be be out for most of the next 4 weeks and may not have much time to review patches.)

Abandoned by a revert of D118493

Yeah; I wish that we don't do this. IIRC even a RISC-V binutils maintainers said the behavior is strange but perhaps people don't have enough motivation to fix that.
We don't really need to copy the behavior as Clang Driver has passed -X.

At least with that this diff shows, the saving is small. I'd hope that we don't toggle -msave-restore for -Os/-Oz. Users can specify -msave-restore themselves.

Since the touched code is still in review, you need to merge the changes into the prior patches.

In D144179#4166909, @mgabka wrote:

In D144179#4146599, @MaskRay wrote:

This looks like introducing a footgun (when something behaves differently from an upstream Clang, it would be difficult for toolchain maintainers to know why).
Why can't your user specify CLANG_CONFIG_FILE_SYSTEM_DIR?

hi @MaskRay,
The reason we wanted to suggest use of environment variable is that the CLANG_CONFIG_FILE_SYSTEM_DIR is only defined at compilation time, after discussing it once again we would rather lean towards introducing an environment variable with similar semantics as CLANG_CONFIG_FILE_SYSTEM_DIR or rather `CLANG_CONFIG_FILE_USER_DIR`, the motivation here is that it will allow to specify the directory to search for config files in a dynamic way, without need to recompile the compiler.
It is for user convenience in situations when they are using a system wide installation in a location where they do not have access right, and the `CLANG_CONFIG_FILE_SYSTEM_DIR and CLANG_CONFIG_FILE_USER_DIR` were not defined at build time.

We realised that environment variables are already used in this area, for example CLANG_NO_DEFAULT_CONFIG, so adding another one is not breaking existing convention.

What do you think about it?