test-suite wrongly using big-endian results

Hi Daniel,

I know you only did a small change to support big/little endian
reference outputs, but maybe you can help me.

I'm running the test-suite on AArch64 and it's correctly detecting
little-endian, even setting the ENDIAN=little on configure and
Makefiles alike, but it still generates "big-endian" from
Makefile.programs.

Here's the first lines of:

sandbox/test-...$ grep -i endian * | less
config.log:| not big endian
config.log:ac_cv_c_bigendian=no
config.log:ENDIAN='little'
config.status:s,@ENDIAN@,|#_!!_#|little,g
configure.log:checking whether byte ordering is bigendian... no
grep: External: Is a directory
Makefile.config:ENDIAN := little

=== So far, so good.

Makefile.programs: -if [ -f
"$(PROJ_SRC_DIR)/$*.reference_output.$(ENDIAN)-endian.$(REFERENCE_OUTPUT_KEY)"
]; then \
Makefile.programs: cp
$(PROJ_SRC_DIR)/$*.reference_output.$(ENDIAN)-endian.$(REFERENCE_OUTPUT_KEY)
$@; \
Makefile.programs: elif [ -f
"$(PROJ_SRC_DIR)/$*.reference_output.$(ENDIAN)-endian" ]; then \
Makefile.programs: cp
$(PROJ_SRC_DIR)/$*.reference_output.$(ENDIAN)-endian $@; \

=== This is correct, and I was assuming ENDIAN := little, as said above.
=== But then you get this on ALL tests in test.log:

test.log:if [ -f
"/home/enevill/llvm/test/test-suite/SingleSource/Regression/C++/EH/ConditionalExpr.reference_output.big-endian."
]; then \
test.log: cp
/home/enevill/llvm/test/test-suite/SingleSource/Regression/C++/EH/ConditionalExpr.reference_output.big-endian.
Output/ConditionalExpr.out-nat; \
test.log: elif [ -f
"/home/enevill/llvm/test/test-suite/SingleSource/Regression/C++/EH/ConditionalExpr.reference_output.big-endian"
]; then \
test.log: cp
/home/enevill/llvm/test/test-suite/SingleSource/Regression/C++/EH/ConditionalExpr.reference_output.big-endian
Output/ConditionalExpr.out-nat; \

Any ideas?

--renato

Hi Renato,

Nothing in particular springs to mind. Is it possible that in the context where this part of the Makefile is running Makefile.config isn’t available?

I’m not sure how this can be completely broken in the fashion you describe though, as it seems like that would make little-endian fail which is what most people are testing.

  • Daniel

Nothing in particular springs to mind. Is it possible that in the context
where this part of the Makefile is running Makefile.config isn't available?

This is a standard lnt run, create in the same way I ran for x86_64
and ARM, so I'm not sure what could be wrong.

I'm not sure how this can be completely broken in the fashion you describe
though, as it seems like that would make little-endian fail which is what
most people are testing.

It is very weird. I got lost twice trying to follow the make files and
autoconf, this is why I ask you, to see if you can think of anything
that would need registering when adding new targets...

I'll keep trying... Thanks!

--renato

I can’t think of whats going on, but to debug I would:

  1. Verify your test suite checkout is clean and doesn’t have any temporary or build products.

  2. Use the ‘lnt runtest nt’ --only-test option for a specific subdir to be able to iterate quickly.

  3. Try adding debug statements to the Makefile to dump the value of ENDIAN, e.g., with:
    $(error “ENDIAN is $ENDIAN”), before and after the include of Makefile.config and Makefile.singlesrc (which includes Makefile.programs).

  • Daniel

Hi Daniel,

Thanks, that did the trick! :wink:

In TargetConfig.mk.in:

#ifdef __LITTLE_ENDIAN__
ENDIAN := little
#else
ENDIAN := big
#endif

Seems like it should be __ORDER_LITTLE_ENDIAN__ to be cross-platform.

Attaching the patch that adds aarch64 (which is identical to the one
applied in LLVM a few days ago) to the config plus change the
TargetConfig.mk.

Shouldn't cause too much disturbance. :wink:

cheers,
--renato

test-suite-aarch64.patch (957 Bytes)

Hi Renato, Daniel,

This is related to a patch I submitted a little while ago (still pending):
  http://llvm-reviews.chandlerc.com/D2760

If accepted, would it make this patch (and a others) unnecessary?

Robert

Hi Robert,

It is, but hijacking your patch a little, why not use
__ORDER_LITTLE_ENDIAN__? Why do we need to create __LITTLE_ENDIAN__ ?

cheers,
--renato

Hi Renato,

You are quite right.

The question is are __LITTLE_ENDIAN__ & __BIG_ENDIAN__ defines becoming 'standard'?
Do we want all targets to have one or the other?
Still waiting for feedback...

Robert

The question is are __LITTLE_ENDIAN__ & __BIG_ENDIAN__ defines becoming 'standard'?

They're redundant with __ORDER_*_ENDIAN__, so I personally wouldn't
even emit them. Even though code might have start depending on them
and it could be hard to deprecate it, I'd still go through. It's not
like people would have to re-implement anything, this is a simple
regex's job.

Do we want all targets to have one or the other?

I'd say every target needs one of the __ORDER_*_ENDIAN__ options, yes,
and also __BYTE_ORDER__ set.

cheers,
--renato

At the very least NetBSD always wants to have __LITTLE_ENDIAN__ /
__BIG_ENDIAN__ as they are much older than __ORDER_*.

Joerg

Right, I didn't know that. I'm fine with both, just wanted to make
sure we were not adding things for the sake of adding things.

--renato