I think MMX code is obscure enough at this point that it doesn't matter much either way. Even less across DSO boundaries. That's why I don't really care either way.

The patch still needs to be generalized to handle all ICE constraints, but it is not blocked by the review for handling the is.constant intrinsic.

In D60942#1500742, @void wrote:

In D60942#1500730, @joerg wrote:

On the frontend side:

• __builtin_constant_p is not expanded correctly in EvalConstant.cpp when used as part of codegen for -O0. This means that trivially unreachable branches are emitted (e.g. with a BR with a literal false as condition),
• The constant evaluator has a general problem with overly pessimistic analysis when it comes to possible side effects. Still have to discuss the correct handling with Richard for this.

GCC has this to say about __builtin_constant_p's behavior:

A return of 0 does not indicate that the value is not a constant, but merely that GCC cannot prove it is a constant with the specified value of the -O option.

When I was modifying our implementation recently, I made it so that if there are no optimizations then we evaluate to 0 in the front-end before code-gen. It might be that those blocks are being emitted, but they should have something like br i1 false, label %true_bb, label %false_bb or equivalent.

On the frontend side:

• __builtin_constant_p is not expanded correctly in EvalConstant.cpp when used as part of codegen for -O0. This means that trivially unreachable branches are emitted (e.g. with a BR with a literal false as condition),
• The constant evaluator has a general problem with overly pessimistic analysis when it comes to possible side effects. Still have to discuss the correct handling with Richard for this.

It does not fix the issues on our side, but pushes them to a different place. It is still an improvement, but the problem is not solved yet.

I'm in the process of testing this, but feedback will take a bit.

In D59816#1442909, @evgeny777 wrote:

don't bother with the 1KB table in both binary and source, just recompute it on the first use

Is single threaded usage guaranteed?

I'd recomment copying the version from libarchive (https://github.com/libarchive/libarchive/blob/master/libarchive/archive_crc32.h):

• don't bother with the 1KB table in both binary and source, just recompute it on the first use.
• at least in the past unrolling the inner loop somewhat helped a lot

LGTM from my perspective at least.

For it to be really useful for the majority of bugs, it would be nice to figure out automatically how to get the preprocessing step done and filter out the # lines afterwards. That part alone significantly cuts down the creduce time.

Why do you exclude clang? I would expect LLVM at this point to not support visibility attributes on XCOFF either?

Well, that was a sample to illustrate the point. A full working (and now failing) example is:

The other problem is that we don't use the CFG machinery to prune dead branches. Consider the x86 in/out instructions: one variant takes an immediate, the other a register. The classic way to deal with that is something like

Can you include a patch for something like (int *)0xdeadbeeeeeef on amd64? That's a valid value for "n", but clearly too large for int. Thanks for looking at this, it is one of the two large remaining show stoppers for the asm constraint check.

Placing .bss.rel.ro before .data doesn't make sense. It forces the content of .bss.rel.ro to embedded into the binary. I also don't really understand the motivation here. Memory mappins on the kernel side are quite cheap.

I'm not in favor of this. It adds overhead for the system compiler and generally makes the logic more complicated. This seems to be another hack around the fact that the driver has no clear notion of "use system runtime" vs "use custom runtime".

Actually, since it used to be part of XSI, I would include it unconditionally and only conditionalize it if a system starts to actually drop it.

The description is certainly not right. IE can be used from dlopen'd objects without problems as long as there is still reserved space around and the data is not initialized non-trivially or the dynamic linker is willing to fix up all threads. The point of the flag is to allow the dynamic linker to make a quick decision without having to scan the relocation table first. A classic user of this was libGL. It also applies to many platforms, not just i386. As such, please update the description before any commit.

Neither GNU ld nor gold implement really useful behavior here. Ideally, every library would be linked with -z defs, but that normally doesn't work because on many systems certain symbols are imported from the main program by default. A good example are the entry points of the dynamic linker. A bad example historically speaking is libwrap. From a user perspective it would be desirable to have a way to say "This symbol will be supplied by someone else" when linking a shared library or potentially even the main binary. That would allow whitelisting intentionally undefined symbols and allow avoiding any symbol resolution dance for shared libraries. A single pass over the symbol tables is still necessary for the sake of knowing the defined symbols and for potential copy relocations, but no further work should be needed.

This seems to imply hard-float support though? Accessing floating point registers when using soft-float is not going to get very far.

I'd really prefer to keep the argv[0] code as is. I'm not sure what that test case is supposed to do, but it seems quite questionable as "check" is not a valid language frontend nor a version suffix. It should not work.

For the clang side, I don't understand why Driver::GetFilePath is not good enough. This shouldn't need all toolchain changes at all.

As discussed with dankm on IRC, I still would like to see the correct behavior going into 8.0, i.e. not change it later. Since this also matters for potential faster implementations later, it seems like a good idea to do it now. The changes are well-localized.

As first step, it goes into the right direction. I would explicitly set --as-needed for all those indirectly loaded objects. If people want to retain the questionable behavior of newer GNU tools, it could be a separate flag so that a final round can warn if an indirectly pulled library is necessary, but that behavior doesn't IMO make much sense. Full version has to look at DT_RUNPATH/DT_RPATH and also --rpath-link.

Right, I'm not aware of anyone but FreeBSD really using the OSABI field. For FreeBSD it was a long standing hack around limitations in the ELF kernel loader. I'm not even sure if FreeBSD use(d) to set the OSABI field for the intermediate object files.

That's the other reason why I find the GCC specification as string prefix confusing. I still say we should just go with mapping of path names and then the order question mostly goes away.

@ruiu: No, it is exactly what you want, since it allows you to point lld into the normal sysroot. Cross-compiling is the default case for the NetBSD toolchain.

Thanks, this looks like a good starting point.

It can certainly make it worse, e.g. if you have one input section with half page size and page alignment followed by another input section with half page size. Depending on the precise size and order, it will fit into one page or two. It's a bit constructured, but gives the general idea.

I think this should factor in any larger-than-normal alignment, otherwise it could easily result in a significant increase in size?

Talking from the perspective of having had to deal with thousands of packages in pkgsrc over the years: it is naive to believe that there isn't a lot of software that calls the linker directly for various reasons. As such, it is very important to have a useful configuration in the linker by default. Such a configuration is by its very nature target specific. This doesn't mean it can't be done in a cross-compiler friendly way, on the contrary. Over the years NetBSD has been pushing its toolchain to be as similar for the native build and a cross-target as reasonable possible. Modulo the build time choices in the config.h sense, the only difference between the native and cross tools is the built-in default of the former.

In D56215#1344279, @krytarowski wrote:

In D56215#1344183, @joerg wrote:

This doesn't seem a reasonable approach at all:

(1) It breaks cross-linking.
(2) It is not correct for any target architecture, e.g. /usr/local/lib certainly doesn't belong on this list and /lib doesn't either.
(3) The correct list depends not only on the target architecture, but also the active emulation.

Is it acceptable to pass all the paths through configure/build phase of lld? It's done this way in GNU ld in the NetBSD distribution. If we need or want to hardcode all the specific paths it will be harder to maintain the proper list and behavior inside lld.

This doesn't seem a reasonable approach at all:

I foundamentally don't understand what you are trying to do. You can either look at the executable from a segment perspective or from a section perspective. But trying to mix the views is bound to give bogus results.

In D55998#1340448, @jankratochvil wrote:

it is in theory possible to create an elf file where only a part of a section would belong to some segment (and another part to a different one).

ELF standard: "An object file segment contains one or more sections.", "An object file segment comprises one or more sections"

This header is used on systems without glibc. So please don't argue about behavior based only on that. Granted, most other libc implementation are less annoying when it comes to free and malloc, but still.

In D55167#1326120, @vitalybuka wrote:

In D55167#1324855, @joerg wrote:

I don't see why this interceptor needs to know about the internals of struct cdbr or struct cdbw at all. They are fully opaque. All memory accessed by users is explicitly sized as argument to the functions or returned with the size.

Then we give up on TSAN check of fieds, or msan check if caller will try to read particular fields

I don't see why this interceptor needs to know about the internals of struct cdbr or struct cdbw at all. They are fully opaque. All memory accessed by users is explicitly sized as argument to the functions or returned with the size.

I'd recomment another pass to reduce the now fully redundant () pairs around % string substitutions, but in principle LGTM.

LGTM

LGTM

LGTM

LGTM

Can you split off the pure modernisation changes like new exception or print ? Those are completely non-contentious changes after all. I generally do not like the range and list related changes as many instances are clear regressions for the 2.x case. filter to list comprehension should IMO be a separate change as well, but those are much less problematic and often an improvement in terms of both performance and readability.

I'm sorry, but it still sounds to me like you want to address badly written build rules by making the driver more complicated. I don't see that is a reasonable goal forward.

I don't understand the point here. Why would you want to include pre-processing-only commands in the compilation database?

Well, I do like the general idea. I would go with a sorted static array and a binary search though. This is not really time critical, but the array version would be smaller in terms of code without involving run-time allocations.

I would actually rule out subshells as well. I see no reason why a test case should have to use them, i.e. compared to just change the working directory back afterwards. As I said, I want to be able to repeat a problematic command manually and interactively as easy as possible. Using non-portable shell features is a problem for that. Depending on state between commands is also annoying. Let's really try to keep it minimal and where necessary, adjust test case guidelines and test cases to simplify things.

As I said: because it makes it harder to rerun parts of the test interactively. Which is a good enough reason to forbid cd - as well as any directory stack operations, IMO.

Personally, I would strongly prefer if the test cases are as explicit as possible. That makes debugging based on the llvm-lit output a lot easier. Ignoring anything else, this feature would make it a lot more tricky to figure out what exactly is going on.

I'm quite against this. "cd -" is a non-portable extension to shell and should never be used in first place.

I think I would enumerate the BSDs here (and Apple) explicitly and not depend on BSD.

Nothing changed. I don't see how catering to the broken libstdc++ self-configuration helps. So no, I still object to this change.

__m68k__ and __ia64__ too, just for completeness.

To summarize the discussion on IRC, from my perspective this change is wrong. PPC as architecture is strongly geared towards PIC code. We used to create quite bad position dependent code and I generally don't see the advantage. I can understand wanting to default to PIE (i.e. position independent code with main binary assumption as far as interception is considered). In other words, the middle end generally tended to create better results for position independent codegen. The default behavior of GCC is ephemeral here, IMO.

Excuse me for bring this up so late, but why do we want to make any such promises? As in: fundamentally, LLVM IR doesn't have any order property on the module level. I have yet so seen reasonable code where the order of functions matters for anything but performance. I've seen a few things that required -funit-at-a-time, most noticable GCC's CRT implementation. But those are all major hacks. So under what premise is it useful to have to even pretend to honor source code order?

Yeah, I would restrict it to just mention that it depends on the target, link time editor and runtime linker. Even the concrete feature set on Linux changes with glibc versions.

I think this is still too optimistic. Full support for ifunc seems to be generally limited to x86. Most other architectures lack even definitions for anonymous ifunc relocations or support proper relaxation only in limited forms. That's especially annoying when looking at static linking.

I find this warning confusing. I find a4 to be perfectly expected. IMO this warning should be applied only, if the effective value of the expression is not the same as in the modulo-n arithmetic. This means that if (-x) is explicitly or implicitly cast to a less wide unsigned type, it should not warn. It would consider a warning for the case of using (-x) if integer promotion rules makes it negative though. The question is, how to best patch around the warning though. What options does MSVC have for that? I.e. what equivalent expressions do not trigger this warning?

Correct. The protected name is double underscore as both suffix and prefix.

Please check the history of the file for some of the problems with the redefinition. I'm quite against this change.

Every system call has a public and internal variant. The former might be replaced by libpthread etc for thread cancellation support, but that's a different topic.